Login using VBScript

Question

I was tasked with re-writing a login VBScript we use on about 50 machines. The original script was hacked together by someone who clearly had no idea what they were doing (multiple lines that literally did nothing, including creating persistent shares, deleting them, then recreating them 4+ times in a ForEach loop)

I don't have any scripting experience in vbs, so I'm assuming there's a good amount of stuff here that should be cleaned up. The ultimate goal is to create up to three mapped drives (P:, Q:, R:) that point at registers in the location. Every site should have an identical set up other than the first two Consts, for ease of re-use.

Option Explicit
Const STORE_NUMBER = "29"
Const NUM_REGS = 3
Const EVENT_SUCCESS = 0
Const EVENT_FAIL = 1
Class Mapping
 Public strLocalDrive
 Public strUNCPath
 Public strPersistent
 Public strUsr
 Public strPas
End Class
Dim alMappings : Set alMappings = CreateObject("System.Collections.ArrayList")
Dim objNetwork : Set objNetwork = WScript.CreateObject("WScript.Network")
Dim oShell : Set oShell = CreateObject("WScript.Shell")
' ## Mapping Objects ##
' POS 1
If NUM_REGS >= 1 Then
 Dim udtPOSOne : Set udtPOSOne = New Mapping
 With udtPOSOne
 .strLocalDrive = "P:"
 .strUNCPath = "\10円.0." & STORE_NUMBER & ".101\dpalm"
 .strPersistent = "False"
 .strUsr = "somename"
 .strPas = "somepass"
 End With
 alMappings.add(udtPOSOne)
End If
' POS 2
If NUM_REGS >= 2 Then
 Dim udtPOSTwo : Set udtPOSTwo = New Mapping
 With udtPOSTwo
 .strLocalDrive = "Q:"
 .strUNCPath = "\10円.0." & STORE_NUMBER & ".102\dpalm"
 .strPersistent = "False"
 .strUsr = "somename"
 .strPas = "somepass"
 End With
 alMappings.add(udtPOSTwo)
End If
' POS 3
If NUM_REGS >= 3 Then
 Dim udtPOSThree : Set udtPOSThree = New Mapping
 With udtPOSThree
 .strLocalDrive = "R:"
 .strUNCPath = "\10円.0." & STORE_NUMBER & ".103\dpalm"
 .strPersistent = "False"
 .strUsr = "somename"
 .strPas = "somepass"
 End With
 alMappings.add(udtPOSThree)
End If
oShell.LogEvent EVENT_SUCCESS, "Mappings built"
Dim udtMapping
For Each udtMapping in alMappings
 ' Unmap the drive if it's currently mapped
 On Error resume Next
 objNetwork.RemoveNetworkDrive udtMapping.strLocalDrive, True
 On Error goto 0
 ' Map the drive
 objNetwork.MapNetworkDrive udtMapping.strLocalDrive, udtMapping.strUNCPath, _
 udtMapping.strPersistent, udtMapping.strUsr, _
 udtMapping.strPas
 ' Windows event logging
 if err.number <> 0 then
 oShell.LogEvent EVENT_FAIL, "Login script failed" & vbcrlf &_
 "Error #: " & err.number & vbcrlf & "Error: " & err.description &_
 "Failed to map " & udtMapping.strUNCPath & " to " & udtMapping.strLocalDrive
 else
 oShell.LogEvent EVENT_SUCCESS, "Login script successfully mapped " &_
 udtMapping.strUNCPath & " to " & udtMapping.strLocalDrive
 end if
Next

• user34073: Please do not edit your question after you have received answers as it will invalidate them: codereview.stackexchange.com/help/on-topic
• Adam Smith: @Hosch250 and nhgrif Thanks! At SO we require you keep the original code intact but can post additions as deemed useful. I'll ask a new question
• user34073: You're welcome. Yes, our rules are somewhat different from SO's.
• nhgrif: @AdamSmith While edits to the code are not allowed after receiving answers, you're more than welcome to edit the plain-English part of your question to more clearly emphasize your areas of primary concern.

Answer 1

Just a quick little note: that chunk here, should be parameterized and moved into a separate procedure:

' POS 1
If NUM_REGS >= 1 Then
 Dim udtPOSOne : Set udtPOSOne = New Mapping
 With udtPOSOne
 .strLocalDrive = "P:"
 .strUNCPath = "\10円.0." & STORE_NUMBER & ".101\dpalm"
 .strPersistent = "False"
 .strUsr = "somename"
 .strPas = "somepass"
 End With
 alMappings.add(udtPOSOne)
End If

...instead of being copied over and repeated 3 times with a different strLocalDrive and strUNCPath.

• Adam Smith: Since I don't know VBScript, can I do that as part of the class instantiation? In Python I would wrap all that logic in the class's __init__ method and simply call it. utdPOSOne = Mapping("P:", "\\\10円.0." + STORE_NUMBER + ".101\\dpalm", "False", "somename", "somepass")
• RubberDuck: You could make it a class method, or you could just declare it much like you declared the class. You want the Sub...End Sub keywords.
• Eric Lippert: @AdamSmith: Unfortunately VBScript does not support parameterized class initialization. Some people do a trick where they have an Init method that returns Me, and then they can say set obj = (new C).Init(arguments) but that is pretty cheesy.

Answer 2

I don't know vbscript even in the slightest, so this review is going to use the Socratic method.

---

Const STORE_NUMBER = "29"

Why is a variable with the word "NUMBER" in the name of the variable actually a string? Should this be an integer?

---

Given:

Const NUM_REGS = 3

Why are we bothering with the following:

If NUM_REGS >= 1 Then

and

If NUM_REGS >= 2 Then

Does Const mean something different in vbscript than it means in every other language?

• Adam Smith: To your first point -- yes that should probably be an integer. I come from Python where "10.0." + 29 + ".101" throws a TypeError. VBScript lets you use the & operator on strings and ints alike. The NUM_REGS constant won't change in the same file, but we're using this same file at various sites that will have different values in NUM_REGS. Rather than change the plumbing for each site, I just wanted to change one constant.
• Eric Lippert: @AdamSmith: In that case it would be a good idea to not use Const to declare the name. Using Const sends a message to the reader "this thing is now and forever this value". Use Const for things that don't change, like pi or the number of protons in an atom of gold. If a thing can change, like a program's version number or the name of your company, then avoid marking it Const.
• Adam Smith: @EricLippert really? That clashes with my view of Const, which is a variable that will remain constant throughout execution of the program/script, rather than an inherent truth about the universe! :)
• nhgrif: @AdamSmith NUM_REGS probably shouldn't be defined within the program. Instead, it should probably be taken as a command line argument.
• Eric Lippert: @AdamSmith: In VBScript your attitude is pretty reasonable, but still I find it jarring to think of a "constant" that changes depending on the context. In languages like C# it is downright dangerous to ever change a constant; if assembly Foo consumes a constant X from assembly Bar, and the author of Bar changes the value of X, then Foo does not "pick up" the change until Foo is recompiled.

Answer 3

Instead of building a mapping class, and then making an array of them, why not just make a sub, and call it directly:

' ## Mapping Objects ##
' POS 1
If NUM_REGS >= 1 Then
 MapDrive "P:", "\10円.0." & STORE_NUMBER & ".101\dpalm", "False", "somename", "somepass"
End If
' POS 2
If NUM_REGS >= 2 Then
 MapDrive "Q:", "\10円.0." & STORE_NUMBER & ".102\dpalm", "False", "somename", "somepass"
End If
' POS 3
If NUM_REGS >= 3 Then
 MapDrive "R:", "\10円.0." & STORE_NUMBER & ".103\dpalm", "False", "somename", "somepass"
End If
oShell.LogEvent EVENT_SUCCESS, "Mappings built"
Sub MapDrive(strLocalDrive, strUNCPath, strPersistent, strUsr, strPas)
 ' Unmap the drive if it's currently mapped
 On Error resume Next
 objNetwork.RemoveNetworkDrive strLocalDrive, True
 On Error goto 0
 ' Map the drive
 objNetwork.MapNetworkDrive strLocalDrive, strUNCPath, _
 strPersistent, strUsr, _
 strPas
 ' Windows event logging
 if err.number <> 0 then
 oShell.LogEvent EVENT_FAIL, "Login script failed" & vbcrlf &_
 "Error #: " & err.number & vbcrlf & "Error: " & err.description &_
 "Failed to map " & strUNCPath & " to " & strLocalDrive
 else
 oShell.LogEvent EVENT_SUCCESS, "Login script successfully mapped " &_
 strUNCPath & " to " & strLocalDrive
 end if
End Sub