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
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
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
Password is meant to be a
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.
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
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:
See, VBA allocates a memory space for that empty string, but not for
vbNullString. And if you call it 5 times...
...you get a new address for it every time.
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
If Confirm = vbNo Then
Password = ""
PasswordLength = 8
Confirm = ""
The two conditions are mutually exclusive, and the
Exit Sub is heuristically unreachable code. It should be one conditional:
If Confirm = vbYes Then
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
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
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.