Please critique this HTA (Change Database Remotely)

H

Highlander

Hello. As a novice in both VBScript and HTML, I usually find code
snippets, tweak 'em to my specifications, then bolt 'em all together to
accomplish what I need. Like in this case. The following script works
fine, but I'm sure it can be streamlined, corrected, and the redundancy
removed.

This HTA will be used to "change the database" of a remote server,
which involves 3 major tasks:

A. Make specific changes to the Registry (using .REG files)
B. Copy a specific Configuration.xml file
C. Restart IIS

The high level functions of the HTA are as follows:

1. Enter or select a server (opens that server's version.ini file once
selected)
2. Select a "release" (determines the source PATH for the
Configuration.xml and .REG files)
3. Select a "database" (determines the source Configuration.xml and
..REG files to be copied)
4. If the target Configuration.xml and .REG files exist and are Read
Only, remove the Read Only attribute
5. Enable the "Change Database" button only when the server, release
and database have been selected.
6. After the "Change Database" button is clicked and the script is
executed:
- reset all variables
- reset text and list boxes
- disable the "Change Database" button

Some obvious things that I imagine can be addressed:

- The conditional IF statement used to enable the "Change Database"
button...I've had to put it in multiple subroutines. Is there a better
way?

- I'm using the sysinternals.com utility PSEXEC to remotely change the
Registry. Is there a better way?

- I'm using the sysinternals.com utility PSEXEC to remotely restart
IIS. Is there a better way?

- I'm using repeated entries of the HTML " " to insert a large
number of spaces in between text and listboxes. Is there a better way?

Any suggestions would be greatly appreciated. Thanks!

- Dave


Watch for word wrap:
===== BEGIN Change_Database_Remotely.hta ====================

<html>

<head>
<title>Change Database Remotely</title>

<HTA:APPLICATION
ID="HTAUI"
APPLICATIONNAME="HTA User Interface"
SCROLL="no"
SINGLEINSTANCE="yes"
WINDOWSTATE="maximized"</head>

<style>
BODY
{
background-color: buttonface;
font-family: Helvetica;
font-size: 12pt;
margin-top: 2px;
margin-left: 8px;
margin-right: 3px;
margin-bottom: 3px;
}

</style>

<SCRIPT language="VBScript">

Dim strServer
Dim strVersionFile
Dim strRelease
Dim strReleasePath
Dim strDatabase
Dim strNewREGFile
Dim strREGPath

Dim s
Dim r
Dim fso
Dim f

Dim sFILE
Dim objShell
Dim objWshScriptExec
Dim objStdOut

Dim objFSO
Dim strREGFileSourcePATH
Dim strREGFile
Dim strTargetPATH
Dim strEXEFile

Dim strConfigPath
Dim strNewConfigFile
Dim strConfigFileSourcePATH
Dim strTargetConfigFile

Dim strREGImportCMD
Dim strIISResetCMD


Sub Window_Onload
self.Focus()
self.ResizeTo 600,500
self.MoveTo 200,50

' Clear all variables in order for the
' "Enable Change Database button" IF statement to work

strServer = ""
strRelease = ""
strDatabase = ""

End sub


Sub ServerButton
strServer = txtServerInput.Value

strCMD = "notepad"
Set objShell = CreateObject("WScript.Shell")

If strServer = "" Then
strServer = cboServerSelection.Value
strVersionFile = "\\" & strServer & "\D$\Program Files\Adss\Patch
Level\Version.ini"
Set objWshScriptExec = objShell.Exec (strCMD & " " & strVersionFile)

Else
strVersionFile = "\\" & strServer & "\D$\Program Files\Adss\Patch
Level\Version.ini"
Set objWshScriptExec = objShell.Exec (strCMD & " " & strVersionFile)

End If

' Enable Change Database button when all criteria is selected
If strServer <> "" and strRelease <> "" and strDatabase <> "" Then
ChangeDatabase_button.Disabled = False

spServer.InnerHTML = strServer

End Sub


Sub ReleaseSelectList
strRelease = ReleaseSelection.Value

If strRelease = "July" Then
strReleasePath = "July 2005 Release\"

Else
strReleasePath = ""

End If

' Enable Change Database button when all criteria is selected
If strServer <> "" and strRelease <> "" and strDatabase <> "" Then
ChangeDatabase_button.Disabled = False

spRelease.InnerHTML = strRelease

End Sub


Sub DBSelectList
strDatabase = DBSelection.Value
strNewREGFile = strDatabase & ".REG"


' Enable Change Database button when all criteria is selected
If strServer <> "" and strRelease <> "" and strDatabase <> "" Then
ChangeDatabase_button.Disabled = False

spDatabase.InnerHTML = strDatabase

End Sub


Sub ChangeDatabase

' Copy REG.EXE and Database REG file to target server temp folder

strREGPath = "\\ServerName\D$\ASISS Team\Tools\Reg Keys\"
strREGFileSourcePATH = strREGPath & strReleasePath & strNewREGFile
strTargetPATH = "\\" & strServer & "\C$\TEMP\"
strEXEFile = "\\ServerName\D$\ASISS Team\Tools\Batch\Reg.exe"


' Modify the Read Only attribute if necessary

If CreateObject("Scripting.FileSystemObject").FileExists(strTargetPATH
& strNewREGFile) Then
' Dim fso, f
Set fso = CreateObject("Scripting.FileSystemObject")
Set f = fso.GetFile(strTargetPATH & strNewREGFile)

' If the first bit of the attributes byte is set to 1,
' the file is Read Only.
' If this is the case, clear the first bit.

If f.attributes and 1 Then
f.attributes = f.attributes - 1
End If

Set objFSO = CreateObject("Scripting.FileSystemObject")
objFSO.CopyFile strREGFileSourcePATH, strTargetPATH, True
objFSO.CopyFile strEXEFile, strTargetPATH, True

Set fso = Nothing
Set f = Nothing

Else

Set objFSO = CreateObject("Scripting.FileSystemObject")
objFSO.CopyFile strREGFileSourcePATH, strTargetPATH, True
objFSO.CopyFile strEXEFile, strTargetPATH, True

End If


' Copy Database CONFIG file to target server Webview folder

strConfigPath = "\\ServerName\D$\ASISS Team\Tools\XML Config Files\"


' Check to see if "TEST" is in the file name
s = "TEST"
r = InStr(strDatabase, s)

If r = 1 Then
strConfigDatabase = right(strDatabase,2)
strNewConfigFile = "Configuration-" & strConfigDatabase & ".xml"

Else
strConfigDatabase = strDatabase
strNewConfigFile = "Configuration-" & strConfigDatabase & ".xml"

End If

strConfigFileSourcePATH = strConfigPath & strReleasePath &
strNewConfigFile
strTargetConfigFile = "\\" & strServer &
"\D$\Inetpub\wwwroot\WebView\Configuration.xml"


' Modify the Read Only attribute if necessary

' Dim fso, f
Set fso = CreateObject("Scripting.FileSystemObject")
Set f = fso.GetFile(strTargetConfigFile)

' If the first bit of the attributes byte is set to 1,
' the file is Read Only.
' If this is the case, clear the first bit.

If f.attributes and 1 Then
f.attributes = f.attributes - 1
End If

Set fso = Nothing
Set f = Nothing

Set objFSO = CreateObject("Scripting.FileSystemObject")
objFSO.CopyFile strConfigFileSourcePATH, strTargetConfigFile, True


' Change Registry
strREGImportCMD = "Psexec" & " \\" & strServer & " " & "cmd /c
C:\TEMP\REG IMPORT"
Set objShell = CreateObject("WScript.Shell")
Set objWshScriptExec = objShell.Exec(strREGImportCMD & " " &
strNewREGFile)


' Restart IIS
strIISResetCMD = "Psexec" & " \\" & strServer & " " & "cmd /c
C:\WINNT\system32\IISRESET /RESTART"

Set objShell = CreateObject("WScript.Shell")
Set objWshScriptExec = objShell.Exec(strIISResetCMD)
Set objStdOut = objWshScriptExec.StdOut


' Display the output of strIISResetCMD

strOutput = objStdOut.ReadAll
MsgBox strOutput, vbInformation, "Change Database Remotely -
COMPLETED."


' Disable Change Database Button

ChangeDatabase_button.Disabled=True


' Clear variables

strServer = ""
strRelease = ""
strDatabase = ""


' Reset all text and list boxes

txtServerInput.value = ""
cboServerSelection.value = ""
DBSelection.value = ""
ReleaseSelection.value = ""

End Sub


' Cleanup
Set strServer = Nothing
Set strVersionFile = Nothing
Set strRelease = Nothing
Set strReleasePath = Nothing
Set strDatabase = Nothing
Set strNewREGFile = Nothing
Set strREGPath = Nothing

Set s = Nothing
Set r = Nothing
Set fso = Nothing
Set f = Nothing

Set sFILE = Nothing
Set objShell = Nothing
Set objWshScriptExec = Nothing
Set objStdOut = Nothing

Set objFSO = Nothing
Set strREGFileSourcePATH = Nothing
Set strREGFile = Nothing
Set strTargetPATH = Nothing
Set strEXEFile = Nothing

Set strConfigPath = Nothing
Set strNewConfigFile = Nothing
Set strConfigFileSourcePATH = Nothing
Set strTargetConfigFile = Nothing

Set strREGImportCMD = Nothing
Set strIISResetCMD = Nothing


</SCRIPT>

<BODY>
<H2 align="center">Change Database Remotely</H2>

<p align="left"><font face="serif" size="4"><b>Enter or select a server
(it's version.ini file

will open):</b></font><br/>

<align="left">
<input type="text" id="txtServerInput"/><br/>
<align="right">
<select id="cboServerSelection">
<option value="0"></option>
<option value="Server1">Server1</option>
<option value="Server2">Server2</option>
<option value="Server3">Server3</option>
<option value="Server4">Server4</option>
<option value="Server5">Server5</option>
<option value="Server6">Server6</option>
</select><br/>
<button onclick="ServerButton">Submit</button>


<p align="left"><font face="serif" size="4"><b>Select a

release:&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Select
a database:</b></font><br/>

<align="left">
<select id="ReleaseSelection" onChange="ReleaseSelectList">
<option value="0"></option>
<option value="July">July</option>
<option value="December">December</option>
</select>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<select
id="DBSelection" onChange="DBSelectList">
<option value="0"></option>
<option value="TESTAJ">TESTAJ</option>
<option value="TESTAP">TESTAP</option>
<option value="TESTAT">TESTAT</option>
<option value="TESTZB">TESTZB</option>
<option value="TESTZE">TESTZE</option>
<option value="TESTZF">TESTZF</option>
<option value="TESTZG">TESTZG</option>
<option value="TESTZH">TESTZH</option>
<option value="TESTZJ">TESTZJ</option>
<option value="TESTZL">TESTZL</option>
<option value="TESTZS">TESTZS</option>
<option value="TESTZT">TESTZT</option>
<option value="TESTZU">TESTZU</option>
<option value="FPT">FPT</option>
<option value="PIONEER">PIONEER</option>
<option value="Production_Training">Production_Training</option>
<option value="PRODUCTION">PRODUCTION</option>
</select><br/>

<p>

<hr>

<span id =spServer></span>

<p>

<span id =spRelease></span>

<p>

<span id =spDatabase></span>

<hr>

<p align="left">

<input id=ChangeDatabaseButton class="button" type="button"
value="Change Database"

name="ChangeDatabase_button" onClick="ChangeDatabase" Disabled=True>


</BODY>
</html>

===== END Change_Database_Remotely.hta ====================
 
A

Al Dunbar

Highlander said:
Hello. As a novice in both VBScript and HTML, I usually find code
snippets, tweak 'em to my specifications, then bolt 'em all together to
accomplish what I need. Like in this case. The following script works
fine, but I'm sure it can be streamlined, corrected, and the redundancy
removed.

This HTA will be used to "change the database" of a remote server,
which involves 3 major tasks:

A. Make specific changes to the Registry (using .REG files)
B. Copy a specific Configuration.xml file
C. Restart IIS
Click to expand...

Sub Window_Onload
self.Focus()
self.ResizeTo 600,500
self.MoveTo 200,50

' Clear all variables in order for the
' "Enable Change Database button" IF statement to work
Click to expand...

Would not be as much of an issue if you avoided using global variables.
strCMD = "notepad"
Click to expand...

recommend using "notepad.exe" instead.
If strServer = "" Then
strServer = cboServerSelection.Value
strVersionFile = "\\" & strServer & "\D$\Program Files\Adss\Patch
Level\Version.ini"
Set objWshScriptExec = objShell.Exec (strCMD & " " & strVersionFile)
Else
strVersionFile = "\\" & strServer & "\D$\Program Files\Adss\Patch
Level\Version.ini"
Set objWshScriptExec = objShell.Exec (strCMD & " " & strVersionFile)
End If
Click to expand...

Suggest extracting the common code from the true and false blocks above,
like this:
If strServer = "" Then
strServer = cboServerSelection.Value
end if
strVersionFile = "\\" & strServer & "\D$\Program Files\Adss\Patch
Level\Version.ini"
Set objWshScriptExec = objShell.Exec (strCMD & " " & strVersionFile)
Click to expand...

/Al
 
H

Highlander

Al said:
Would not be as much of an issue if you avoided using global variables.
Click to expand...

Since the "Enable Change Database button" IF statement has to be in all
3 subroutines (ServerButton, ReleaseSelectList and DBSelectList), I had
to make the 3 variables (strServer, strRelease and strDatabase) global.
recommend using "notepad.exe" instead.

Done.


Suggest extracting the common code from the true and false blocks above,
like this:


/Al
Click to expand...

Done.
Extracting the common code from the true and false blocks worked great.
I used this same logic in other areas of the script and overall I
trimmed between 25 and 30 lines. Thanks Al!

Any suggestions on my remaining questions?

- The conditional IF statement used to enable the "Change Database"
button...I've had to put it in multiple subroutines. Is there a better
way?

- I'm using the sysinternals.com utility PSEXEC to remotely change the
Registry. Is there a better way?

- I'm using the sysinternals.com utility PSEXEC to remotely restart
IIS. Is there a better way?

- I'm using repeated entries of the HTML "&nbsp;" to insert a large
number of spaces in between text and listboxes. Is there a better way?

- Dave

Modified script:

(Watch for word wrap)
===== BEGIN Change_Database_Remotely.hta ====================

<html>

<head>
<title>Change Database Remotely</title>

<HTA:APPLICATION
ID="HTAUI"
APPLICATIONNAME="HTA User Interface"
SCROLL="no"
SINGLEINSTANCE="yes"
WINDOWSTATE="maximized"</head>

<style>
BODY
{
background-color: buttonface;
font-family: Helvetica;
font-size: 12pt;
margin-top: 2px;
margin-left: 8px;
margin-right: 3px;
margin-bottom: 3px;
}

</style>

<SCRIPT language="VBScript">

Dim strServer
Dim strVersionFile
Dim strRelease
Dim strReleasePath
Dim strDatabase
Dim strNewREGFile
Dim strREGPath

Dim s
Dim r
Dim FSO
Dim f

Dim sFILE
Dim objShell
Dim objWshScriptExec
Dim objStdOut

Dim strREGFileSourcePATH
Dim strREGFile
Dim strTargetPATH
Dim strEXEFile

Dim strConfigPath
Dim strNewConfigFile
Dim strConfigFileSourcePATH
Dim strTargetConfigFile

Dim strREGImportCMD
Dim strIISResetCMD

Sub Window_Onload
self.Focus()
self.ResizeTo 600,500
self.MoveTo 200,50

' Clear all variables in order for the
' "Enable Change Database button" IF statement to work

strServer = ""
strRelease = ""
strDatabase = ""

End sub

Sub ServerButton
strServer = txtServerInput.Value

strCMD = "notepad.exe"
Set objShell = CreateObject("WScript.Shell")

If strServer = "" Then
strServer = cboServerSelection.Value
End if

strVersionFile = "\\" & strServer & "\D$\Program Files\Adss\Patch
Level\Version.ini"
Set objWshScriptExec = objShell.Exec (strCMD & " " & strVersionFile)

' Enable Change Database button when all criteria is selected
If strServer <> "" and strRelease <> "" and strDatabase <> "" Then
ChangeDatabase_button.Disabled = False

' Display server name in HTA window
spServer.InnerHTML = strServer

End Sub

Sub ReleaseSelectList
strRelease = ReleaseSelection.Value

If strRelease = "July" Then
strReleasePath = "July 2005 Release\"

Else
strReleasePath = ""

End If

' Enable Change Database button when all criteria is selected
If strServer <> "" and strRelease <> "" and strDatabase <> "" Then
ChangeDatabase_button.Disabled = False

' Display release in HTA window
spRelease.InnerHTML = strRelease

End Sub

Sub DBSelectList
strDatabase = DBSelection.Value
strNewREGFile = strDatabase & ".REG"

' Enable Change Database button when all criteria is selected
If strServer <> "" and strRelease <> "" and strDatabase <> "" Then
ChangeDatabase_button.Disabled = False

' Display database in HTA window
spDatabase.InnerHTML = strDatabase

End Sub

Sub ChangeDatabase
' Copy REG.EXE and Database REG file to target server temp folder

strREGPath = "\\ServerName\D$\ASISS Team\Tools\Reg Keys\"
strREGFileSourcePATH = strREGPath & strReleasePath & strNewREGFile
strTargetPATH = "\\" & strServer & "\C$\TEMP\"
strEXEFile = "\\ServerName\D$\ASISS Team\Tools\Batch\Reg.exe"

Set FSO = CreateObject("Scripting.FileSystemObject")

' If file exists, remove the Read Only attribute if necessary
If CreateObject("Scripting.FileSystemObject").FileExists(strTargetPATH
& strNewREGFile) Then

Set f = fso.GetFile(strTargetPATH & strNewREGFile)

' If the first bit of the attributes byte is set to 1,
' the file is Read Only.
' If this is the case, clear the first bit.

If f.attributes and 1 Then
f.attributes = f.attributes - 1
End If
End If

FSO.CopyFile strREGFileSourcePATH, strTargetPATH, True
FSO.CopyFile strEXEFile, strTargetPATH, True

' Copy Database CONFIG file to target server Webview folder
strConfigPath = "\\ServerName\D$\ASISS Team\Tools\XML Config Files\"

' Check to see if "TEST" is in the file name
s = "TEST"
r = InStr(strDatabase, s)

If r = 1 Then
strConfigDatabase = right(strDatabase,2)

Else
strConfigDatabase = strDatabase

End If

strNewConfigFile = "Configuration-" & strConfigDatabase & ".xml"

strConfigFileSourcePATH = strConfigPath & strReleasePath &
strNewConfigFile
strTargetConfigFile = "\\" & strServer &
"\D$\Inetpub\wwwroot\WebView\Configuration.xml"

' Remove the Read Only attribute if necessary
Set f = fso.GetFile(strTargetConfigFile)

' If the first bit of the attributes byte is set to 1,
' the file is Read Only.
' If this is the case, clear the first bit.

If f.attributes and 1 Then
f.attributes = f.attributes - 1
End If

FSO.CopyFile strConfigFileSourcePATH, strTargetConfigFile, True

Set objShell = CreateObject("WScript.Shell")

' Change Registry
strREGImportCMD = "Psexec" & " \\" & strServer & " " & "cmd /c
C:\TEMP\REG IMPORT"
Set objWshScriptExec = objShell.Exec(strREGImportCMD & " " &
strNewREGFile)

' Restart IIS
strIISResetCMD = "Psexec" & " \\" & strServer & " " & "cmd /c
C:\WINNT\system32\IISRESET /RESTART"
Set objWshScriptExec = objShell.Exec(strIISResetCMD)

' Display the output of strIISResetCMD
Set objStdOut = objWshScriptExec.StdOut
strOutput = objStdOut.ReadAll
MsgBox strOutput, vbInformation, "Change Database Remotely -
COMPLETED."

' Disable Change Database Button
ChangeDatabase_button.Disabled=True

' Clear variables
strServer = ""
strRelease = ""
strDatabase = ""

' Reset all text and list boxes
txtServerInput.value = ""
cboServerSelection.value = ""
DBSelection.value = ""
ReleaseSelection.value = ""

End Sub

' Cleanup
Set strServer = Nothing
Set strVersionFile = Nothing
Set strRelease = Nothing
Set strReleasePath = Nothing
Set strDatabase = Nothing
Set strNewREGFile = Nothing
Set strREGPath = Nothing

Set s = Nothing
Set r = Nothing
Set FSO = Nothing
Set f = Nothing

Set sFILE = Nothing
Set objShell = Nothing
Set objWshScriptExec = Nothing
Set objStdOut = Nothing

Set strREGFileSourcePATH = Nothing
Set strREGFile = Nothing
Set strTargetPATH = Nothing
Set strEXEFile = Nothing

Set strConfigPath = Nothing
Set strNewConfigFile = Nothing
Set strConfigFileSourcePATH = Nothing
Set strTargetConfigFile = Nothing

Set strREGImportCMD = Nothing
Set strIISResetCMD = Nothing

</SCRIPT>

<BODY>
<H2 align="center">Change Database Remotely</H2>

<p align="left"><font face="serif" size="4"><b>Enter or select a server
(it's version.ini file will open):</b></font><br/>

<align="left">
<input type="text" id="txtServerInput"/><br/>
<align="right">
<select id="cboServerSelection">
<option value="0"></option>
<option value="Server1">Server1</option>
<option value="Server2">Server2</option>
<option value="Server3">Server3</option>
<option value="Server4">Server4</option>
<option value="Server5">Server5</option>
<option value="Server6">Server6</option>
</select><br/>
<button onclick="ServerButton">Submit</button>


<p align="left"><font face="serif" size="4"><b>Select a
release:&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Select
a database:</b></font><br/>

<align="left">
<select id="ReleaseSelection" onChange="ReleaseSelectList">
<option value="0"></option>
<option value="July">July</option>
<option value="December">December</option>
</select>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<select
id="DBSelection" onChange="DBSelectList">
<option value="0"></option>
<option value="TESTAJ">TESTAJ</option>
<option value="TESTAP">TESTAP</option>
<option value="TESTAT">TESTAT</option>
<option value="TESTZB">TESTZB</option>
<option value="TESTZE">TESTZE</option>
<option value="TESTZF">TESTZF</option>
<option value="TESTZG">TESTZG</option>
<option value="TESTZH">TESTZH</option>
<option value="TESTZJ">TESTZJ</option>
<option value="TESTZL">TESTZL</option>
<option value="TESTZS">TESTZS</option>
<option value="TESTZT">TESTZT</option>
<option value="TESTZU">TESTZU</option>
<option value="FPT">FPT</option>
<option value="PIONEER">PIONEER</option>
<option value="Production_Training">Production_Training</option>
<option value="PRODUCTION">PRODUCTION</option>
</select><br/>

<p>

<hr>

<span id =spServer></span>

<p>

<span id =spRelease></span>

<p>

<span id =spDatabase></span>

<hr>

<p align="left">

<input id=ChangeDatabaseButton class="button" type="button"
value="Change Database" name="ChangeDatabase_button"
onClick="ChangeDatabase" Disabled=True>


</BODY>
</html>

===== END Change_Database_Remotely.hta ====================
 
A

Al Dunbar

Highlander said:
Since the "Enable Change Database button" IF statement has to be in all
3 subroutines (ServerButton, ReleaseSelectList and DBSelectList), I had
to make the 3 variables (strServer, strRelease and strDatabase) global.
Click to expand...

As you wrote it, perhaps, however I try to avoid global variables at almost
all costs.

Done.
Extracting the common code from the true and false blocks worked great.
I used this same logic in other areas of the script and overall I
trimmed between 25 and 30 lines. Thanks Al!
Click to expand...

The issue is not trimming lines; the issue is the simplicity and
maintainability of the code.
Any suggestions on my remaining questions?
Click to expand...

I didn't answer them all before because you posted a script of nearly 400
lines. As it is, and without doing an exhaustive (and exhausting) analysis
of the whole thing (which would require a lot of deductive reasoning to
figure out), all I can do is make a few suggestions based on what I see in
your code (see a few more way below...). Unfortunately, it might be that a
better next version would actually do things differently.
- The conditional IF statement used to enable the "Change Database"
button...I've had to put it in multiple subroutines. Is there a better
way?
Click to expand...

You could put that code in a small subroutine and call it from wherever you
need to. This would localize your references to these global variables.
- I'm using the sysinternals.com utility PSEXEC to remotely change the
Registry. Is there a better way?
Click to expand...

There are other ways. What is it that you might think needs improvement in
this aspect of your script?
- I'm using the sysinternals.com utility PSEXEC to remotely restart
IIS. Is there a better way?
Click to expand...


There may be other ways. What is it that you might think needs improvement
in this aspect of your script?
- I'm using repeated entries of the HTML "&nbsp;" to insert a large
number of spaces in between text and listboxes. Is there a better way?
Click to expand...

Not really a vbscript question, but it depends on why you need whitespace.
To be honest, I am not going to run your HTA to see what it looks like, but
you might be able to produce a better laid out window using HTML tables.

Here are some more suggestions for you...

- Use a more typical indentation style. For example, the "sub" and "end sub"
statements should be indented less than what they enclose.

- this code:

' If the first bit of the attributes byte is set to 1,
' the file is Read Only.
' If this is the case, clear the first bit.

If f.attributes and 1 Then
f.attributes = f.attributes - 1
End If

would be more correctly coded as:

f.attributes = f.attributes or -2

- in your "cleanup section" at the end, you seem to set all variables to
nothing. This is not required of any variable that does not contain an
object. And of those variables containing objects, not all need be dismissed
this way.

/Al
 
H

Highlander

Al said:
As you wrote it, perhaps, however I try to avoid global variables at almost
all costs.
Click to expand...

That's my point - is there another way to write it? You mention below
that there may be other ways, i.e. put that code in a small subroutine
and call it from wherever you need to.
The issue is not trimming lines; the issue is the simplicity and
maintainability of the code.
Click to expand...

That makes sense. I would imagine though that trimming the code and
removing redundancy where you can is part of making it simpler and
easier to maintain.

Ideally a script would be written "right" in the first place, the right
logic, right commands, right format, etc. Simplicity and
maintainability would be there from the outset. In my case I don't have
the experience and never learned VBScript from the ground up, the
basics, the guidelines, etc. I create scripts in a crude way, by
bolting together code snippets from many different sources, and then
attempting to pare it down so that it has some semblance of efficiency,
simplicity and maintainability. That's why I value your opinion (as
well as other helpful responders on Usenet) highly. You help me get
there.
I didn't answer them all before because you posted a script of nearly 400
lines. As it is, and without doing an exhaustive (and exhausting) analysis
of the whole thing (which would require a lot of deductive reasoning to
figure out), all I can do is make a few suggestions based on what I see in
your code (see a few more way below...). Unfortunately, it might be that a
better next version would actually do things differently.


You could put that code in a small subroutine and call it from wherever you
need to. This would localize your references to these global variables.
Click to expand...

Can you give me an example of what this small subroutine would look
like, and how I would call it?
There are other ways. What is it that you might think needs improvement in
this aspect of your script?


There may be other ways. What is it that you might think needs improvement
in this aspect of your script?
Click to expand...

I just wondered if there is functionality within VBScript syntax that
will perform the same tasks. Although shelling out and using PSEXEC
works fine in this script, I just considered that using inherent
VBScript capability (if it exists) is "cleaner" than shelling out and
using 3rd party utilities.
Not really a vbscript question, but it depends on why you need whitespace.
To be honest, I am not going to run your HTA to see what it looks like, but
you might be able to produce a better laid out window using HTML tables.

Here are some more suggestions for you...

- Use a more typical indentation style. For example, the "sub" and "end sub"
statements should be indented less than what they enclose.

- this code:

' If the first bit of the attributes byte is set to 1,
' the file is Read Only.
' If this is the case, clear the first bit.

If f.attributes and 1 Then
f.attributes = f.attributes - 1
End If

would be more correctly coded as:

f.attributes = f.attributes or -2
Click to expand...

I tried "f.attributes = f.attributes or -2" and it didn't clear the
Read Only bit; it actually gave the file hidden and system attributes.
- in your "cleanup section" at the end, you seem to set all variables to
nothing. This is not required of any variable that does not contain an
object. And of those variables containing objects, not all need be dismissed
this way.
Click to expand...

Since I've seen the "set variable = nothing" in many scripts, and have
been told that this is done to release memory, I assumed that it needed
to be done with every variable. If this isn't the case, what's the
criteria for when it should be used for a given variable?
/Al
Click to expand...

Thanks for your time Al.

- Dave
 
M

McKirahan

[snip]
Since I've seen the "set variable = nothing" in many scripts, and have
been told that this is done to release memory, I assumed that it needed
to be done with every variable. If this isn't the case, what's the
criteria for when it should be used for a given variable?
Click to expand...

Objects are "Set" and may be assigned to "Nothing" when through.

For example,

Set FSO = CreateObject("Scripting.FileSystemObject")
....
Set FSO = Nothing
 
H

Highlander

McKirahan said:
[snip]
Since I've seen the "set variable = nothing" in many scripts, and have
been told that this is done to release memory, I assumed that it needed
to be done with every variable. If this isn't the case, what's the
criteria for when it should be used for a given variable?
Click to expand...

Objects are "Set" and may be assigned to "Nothing" when through.

For example,

Set FSO = CreateObject("Scripting.FileSystemObject")
...
Set FSO = Nothing
Click to expand...

I did this with every object that was "Dimmed". So you're saying that I
only need to take every object that was "Set" and assign them to
"Nothing" when through?
 
M

McKirahan

Highlander said:
[snip]
Since I've seen the "set variable = nothing" in many scripts, and have
been told that this is done to release memory, I assumed that it needed
to be done with every variable. If this isn't the case, what's the
criteria for when it should be used for a given variable?
Click to expand...

Objects are "Set" and may be assigned to "Nothing" when through.

For example,

Set FSO = CreateObject("Scripting.FileSystemObject")
...
Set FSO = Nothing
Click to expand...

I did this with every object that was "Dimmed". So you're saying that I
only need to take every object that was "Set" and assign them to
"Nothing" when through?
Click to expand...

Exactly.

http://ns7.webmasters.com/caspdoc/html/vbscript_set_statement.htm
 
A

Al Dunbar

McKirahan said:
Highlander said:
[snip]

Since I've seen the "set variable = nothing" in many scripts, and
have
been told that this is done to release memory, I assumed that it needed
to be done with every variable. If this isn't the case, what's the
criteria for when it should be used for a given variable?

Objects are "Set" and may be assigned to "Nothing" when through.

For example,

Set FSO = CreateObject("Scripting.FileSystemObject")
...
Set FSO = Nothing
Click to expand...

I did this with every object that was "Dimmed". So you're saying that I
only need to take every object that was "Set" and assign them to
"Nothing" when through?
Click to expand...

Exactly.
Click to expand...

Note, however, that there are cases where it is not actually necessary to
set an object variable to nothing. This happens automatically when the
variable goes out of scope. Also, simply setting an object variable to
nothing may NOT be all that is required to properly clean things up - think
of the need to .close an output file, for one obvious example.

/Al
 
A

Al Dunbar

Highlander said:
That's my point - is there another way to write it?
Click to expand...

For any given possible program there are many ways to write it. What I am
suggesting is not to mechanically do something to the code so that it no
longer relies on global variables. The only way to really achieve a change
in this direction is to re-think your overall coding strategy to avoid this
potentially dangerous dependency. The reason for doing this is primarily to
improve maintainability, but having me show you how I would have written
this particular script would not likely be too useful for you.
You mention below
that there may be other ways, i.e. put that code in a small subroutine
and call it from wherever you need to.


That makes sense. I would imagine though that trimming the code and
removing redundancy where you can is part of making it simpler and
easier to maintain.

Ideally a script would be written "right" in the first place, the right
logic, right commands, right format, etc.
Click to expand...

Ideally, yes, however, one way to avoid problems is to never make the
assumption that your code is correct. In fact, almost all code could
probably have been written better. It just will not be written better until
the writer of the code learns to believe that "almost all code could
probably have been written better".
Simplicity and
maintainability would be there from the outset. In my case I don't have
the experience and never learned VBScript from the ground up, the
basics, the guidelines, etc. I create scripts in a crude way, by
bolting together code snippets from many different sources, and then
attempting to pare it down so that it has some semblance of efficiency,
simplicity and maintainability.
Click to expand...

That is a completely accurate description of how many of us have learned to
script (or to program). And it is done this way, IMHO, because there is not
enough emphasis placed on how to structure code to make it simpler. Some of
the standard exhortations (declare all variables at the top...) Are fine to
just get a person going and functional, but what we really need is an
approach to making the code truly modular and reusable. I do it my way, but
that is not the only way.
That's why I value your opinion (as
well as other helpful responders on Usenet) highly. You help me get
there.
Click to expand...

Thanks - on behalf of all those other helpful people out there :).
Can you give me an example of what this small subroutine would look
like, and how I would call it?
Click to expand...

OK, so, you have instances of code like this:

' Enable Change Database button when all criteria is selected
If strServer <> "" and strRelease <> "" and strDatabase <> "" Then
ChangeDatabase_button.Disabled = False

If you leave the variables as globals for now, you could replace the above
statement with:

AdjustDBbutton

and create this sub:

sub AdjustDBbutton()
If strServer <> "" and strRelease <> "" and strDatabase <> "" Then
ChangeDatabase_button.Disabled = False
end if

Not much saved there, however, it is less likely that you would introduce an
undetected error by mistyping the long if statement.

The trouble with global variables is that any of your code could
unintentionally alter those variables, and it would be difficult to figure
out where this is happening. This is where some of the concepts of object
oriented programming come in: if the only access to some persistent data
storage is through a set of routines, then that set of routines can be
designed to help prevent errors.

Short of creating classes and instantiating objects from them, this can be
simulated with a pair of functions for each such "value". For example, the
strServer concept could be managed with code such as:

private STRSERVER_PERSISTENT
function strServer()
strServer = STRSERVER_PERSISTENT
end function
sub setstrServer(value)
STRSERVER_PERSISTENT = value
end sub

Then you would change all instances of "strServer = some_value" to
"setstrServer some_value", and all other instances of "strServer" would
remain unchanged in your code, i.e.:

strServer = cboServerSelection.Value

would become

setstrServer cboServerSelection.Value

Then any of the references to the old strServer variable (which should no
longer be DIM'd anywhere) will receive the stored value as the return value
of a function instead of directly from a global variable.

Seems like a lot of work, no? Well, I basically did that in one project I
was working on, and once I had made the changes I found things suddenly got
simpler. As an aside, note that the "STRSERVER_PERSISTENT" variable declared
by the "private STRSERVER_PERSISTENT" statement *is* a global variable. But
it is one that, according to my convention, aught not to be referenced
outside of the function and sub pair that "owns" it.

I'm not suggesting that you should mechanically make the equivalent changes
wherever you have a global variable. As I started down that road, I found
that there were other ways to reduce my dependence on the "global" aspect of
global variables. If your coding approach rules against using global
variables in the first place, there will likely be fewer instances where
they need be simulated as above.
I just wondered if there is functionality within VBScript syntax that
will perform the same tasks. Although shelling out and using PSEXEC
works fine in this script, I just considered that using inherent
VBScript capability (if it exists) is "cleaner" than shelling out and
using 3rd party utilities.
Click to expand...

I understand and agree. Unfortunately, some of the things that are simple to
accomlish by calling external utilities require a pile of coding to
accomplish in more VBScript-exclusive ways. One good example is changing
NTFS permissions - a whole lot easier to call cacls.exe than to write the
corresponding vsbcript code from the ground up.

As to how to remotely start IIS, I have never had to do this so have not
researched it. Changing the registry remotely could likely be done with WMI,
but that can be a whole 'nuther can of worms, and one I have lost the opener
for.

One final (for now) hint: you seem to have the ability to look at a problem
and work out a solution for it in code - an admirable skill, in fact. Use
that same skill on the following problem: "here is how I code, how can I do
better?". And don't forget that, in addition to deductive reasoning, you can
use experimentation ("let's try this a couple of ways and see which works
best, is simplest, is easiest to understand, is harder to get wrong, etc."),
and informal statistical analysis. For example, you could informally track
how much actual trouble you have working on a couple of different projects
and correlate that with the extent to which variables are global, the
proportion of the code that is in script-level scope versus functions and
subs, and etc, this might help you figure out why some coding models are
better than others.

/Al
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

I dont get this. Please help me!! 2
Survey details won't go through using php, ajax, Mysql 0
Create Pulldown Menus for Month/Day/Year in HTA 13
Collect Excel Data from Website 5
Can't get rid of this error. ListView. Please, need help. 4
ASP page to validate username with AD 0
cretae ASP page to validate username with AD 5
Add record delete function to an Access Database List - Help Please 0

Members online

Total: 32 (members: 1, guests: 31)
Robots: 450

Forum statistics

Threads
473,769
Messages
2,569,576
Members
45,054
Latest member
LucyCarper

Latest Threads

Top