Basic Password System in Excel VBA

Question

I have created a basic login system that asks the user for a password. Here's a list of what it does:

• When you run the code it will open an Inputbox asking you for a password.
• There is one criteria: You have to enter a password of 8 characters. If you enter a password with less or more, it will give you an error message.
• If you leave the Inputbox blank or press Cancel, it will switch over to a Yes or No msgbox asking you if you want to quit the macro. Yes will close and No, will bring you back to the Inputbox.

I believe that there are many ways to optimize the code. Are there any suggestions?

Sub Userlogin(Password, ExitApplication)

Dim Confirm
Dim PasswordLength As Integer

' Password System
If Password = "" Then

    ' Loop until Password is correct.
    Do While Password = ""
        Password = InputBox("Enter your Password:", "Login")
        PasswordLength = Len(Password)

        ' Asks the user if they want to close the application.
        If Password = "" Then
            Confirm = MsgBox("Are you sure you want to close this application?", vbYesNo, "Confirm")
        End If

            If Confirm = vbYes Then
                ExitApplication = True
                Exit Do
                Exit Sub
            End If

            If Confirm = vbNo Then
                Password = ""
                PasswordLength = 8
                Confirm = ""
            End If

        ' Checks if the Password is 8 characters.
        If Not PasswordLength = 8 Then
            MsgBox "The Password must be 8 characters long." & vbNewLine & vbNewLine & "Please Try again.", vbCritical, "Password Incorrect"
            Password = ""
        End If

    Loop

End If

End Sub

Main Sub is here:

Sub MainSub()

' Standard Dim's
Dim Username As String
Dim Password As String

Dim ExitApplication As Boolean

'_______________________________________________________________________________

' Set Values

ExitApplication = False
Username = CreateObject("WScript.Network").Username

'_______________________________________________________________________________

Call Userlogin(Password, ExitApplication)
    If ExitApplication = True Then
        Exit Sub
    End If

End Sub

Answer 1

It should be a function. ExitApplication shouldn't need to exist, it ought to be the function's return value.

Function Userlogin(Password, ExitApplication) As Boolean

If the function returns True, we proceed; if the function returns False, the calling code can act accordingly, and exit the application.

Kudos for declaring all the variables that you're using. If the module doesn't specify Option Explicit at the top, add it, and make it a habit: that will make VBA refuse to compile code that contains typos (/undeclared variables).

This is a bad, misleading comment:

' Loop until Password is correct.
Do While Password = ""

The comment says one thing, the code says another. Remove the comment, the only truth here is that of the code. Same with any comment that narrates what the code does: useful comments explain why the code does what it does, and let the code speak for itself about what it's doing.

Your members are implicitly Public. If MainSub is meant to be called from the outside, or as a macro, then make it explicitly Public, and then if UserLogin is meant to only ever be called from MainSub, then make it Private.

Your parameters are implicitly passed ByRef. In many languages including VB.NET, parameters are passed by value by default; it's a good idea to specify ByRef explicitly when you mean to pass them by reference.

The parameters are also implicitly Variant, but Password is meant to be a String and ExitApplication is meant to be a Boolean; it's also a good idea to consistently specify a type, even when that type is Variant, and especially when that type is NOT Variant: that way you'll allocate only the memory space you need to allocate; no more, no less.

The Call keyword / explicit Call syntax is obsolete, and only supported for backward compatibility. Instead of this:

Call UserLogin(Password, ExitApplication)

You can do:

UserLogin Password, ExitApplication

You're not using explicit Call for MsgBox and InputBox function calls; there's no need to use an explicit Call syntax for your own functions either.

You use a lot of "" empty string literals; the vbNullString built-in constant is more efficient, since it's a null string pointer - as the following code in immediate pane demonstrate:

?StrPtr(""), StrPtr(vbNullString)
 456810912     0 

See, VBA allocates a memory space for that empty string, but not for vbNullString. And if you call it 5 times...

?StrPtr(""), StrPtr(vbNullString)
 458095304     0 
 458097512     0 
 167896456     0 
 241904984     0 
 456810912     0 

...you get a new address for it every time.

The InputBox result length-check is fine in this case, because an empty string is deemed invalid input - but the correct way to determine whether the user actually cancelled-out of the input box or meant to supply an empty string, is to verify the StrPtr string pointer value of the result: if that value is 0, the user cancelled the InputBox. If that value isn't 0, the user meant to supply an empty string.

This is interesting:

    If Confirm = vbYes Then
        ExitApplication = True
        Exit Do
        Exit Sub
    End If

    If Confirm = vbNo Then
        Password = ""
        PasswordLength = 8
        Confirm = ""
    End If

The two conditions are mutually exclusive, and the Exit Sub is heuristically unreachable code. It should be one conditional:

If Confirm = vbYes Then
    '...
Else
    '...
End If

It's not clear why Confirm is assigned to an empty string; it's a Variant/VbMsgBoxResult variable (implicitly declared as a Variant), so if anything it should be reset to 0, since VbMsgBoxResult is an enum type, and enum values are really just Long constants.

It looks particularly weird to see Password = "" immediately followed by PasswordLength = 8 - seems you're giving different meanings to PasswordLength. At one point it stands for the expected length, and at another it stands for the user input length. This is confusing.

Remove it. All you really need is this:

Const RequiredLength As Integer = 8

And then:

If Len(Password) <> RequiredLength Then

It reads much clearer, and removes the magic value 8 from your code, too; the string message should also be using that RequiredLength constant, so that if it ever needs to change to 12 (it probably won't, but don't write code with the assumption that requirements never change - that's a terrible mistake to make), the message isn't going to be misleading.