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

Please do not edit your question after you have received answers as it will invalidate them: codereview.stackexchange.com/help/on-topic — user34073
– user34073, Commented Mar 12, 2015 at 22:13
@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 — Adam Smith
– Adam Smith, Commented Mar 12, 2015 at 22:15
You're welcome. Yes, our rules are somewhat different from SO's. — user34073
– user34073, Commented Mar 12, 2015 at 22:17
@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. — nhgrif
– nhgrif, Commented Mar 12, 2015 at 23:17

Mathieu Guindon · Accepted Answer · 2015-03-12 20:47:53Z

7

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.

answered Mar 12, 2015 at 20:47

Mathieu Guindon

75.6k18 gold badges195 silver badges469 bronze badges

\$\begingroup\$ 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") \$\endgroup\$

Adam Smith
– Adam Smith

2015-03-12 20:51:03 +00:00
Commented Mar 12, 2015 at 20:51
\$\begingroup\$ 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. \$\endgroup\$

RubberDuck
– RubberDuck

2015-03-12 21:01:06 +00:00
Commented Mar 12, 2015 at 21:01
\$\begingroup\$ @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. \$\endgroup\$

Eric Lippert
– Eric Lippert

2015-03-12 23:02:07 +00:00
Commented Mar 12, 2015 at 23:02

Add a comment |

nhgrif · Accepted Answer · 2015-03-12 21:28:07Z

7

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?

answered Mar 12, 2015 at 21:28

nhgrif

25.4k3 gold badges64 silver badges129 bronze badges

\$\begingroup\$ 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. \$\endgroup\$

Adam Smith
– Adam Smith

2015-03-12 21:36:15 +00:00
Commented Mar 12, 2015 at 21:36
2

\$\begingroup\$ @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. \$\endgroup\$

Eric Lippert
– Eric Lippert

2015-03-12 22:50:59 +00:00
Commented Mar 12, 2015 at 22:50
\$\begingroup\$ @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! :) \$\endgroup\$

Adam Smith
– Adam Smith

2015-03-12 22:56:46 +00:00
Commented Mar 12, 2015 at 22:56
1

\$\begingroup\$ @AdamSmith NUM_REGS probably shouldn't be defined within the program. Instead, it should probably be taken as a command line argument. \$\endgroup\$

nhgrif
– nhgrif

2015-03-12 22:58:56 +00:00
Commented Mar 12, 2015 at 22:58
3

\$\begingroup\$ @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. \$\endgroup\$

Eric Lippert
– Eric Lippert

2015-03-12 22:58:56 +00:00
Commented Mar 12, 2015 at 22:58

Add a comment |

James · Accepted Answer · 2015-03-13 06:27:07Z

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

Stack Exchange Network

Login using VBScript

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Login using VBScript

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions