|
|
Posted by Al Dunbar on 10/11/53 11:33
"Highlander" <tron9901@msn.com> wrote in message
news:1133322022.454485.96000@f14g2000cwb.googlegroups.com...
> Al Dunbar wrote:
>> "Highlander" <tron9901@msn.com> wrote in message
>> news:1133209896.037785.269910@o13g2000cwo.googlegroups.com...
<snip>
>> > 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
>>
>> Would not be as much of an issue if you avoided using global variables.
>
> 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.
As you wrote it, perhaps, however I try to avoid global variables at almost
all costs.
<snip>
> 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!
The issue is not trimming lines; the issue is the simplicity and
maintainability of the code.
> Any suggestions on my remaining questions?
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?
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?
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?
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 " " to insert a large
> number of spaces in between text and listboxes. Is there a better way?
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
[Back to original message]
|