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 returnsTrue, we proceed; if the function returnsFalse, 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 specifyOption 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 narrateswhat the code does: useful comments explainwhy the code does what it does, and let the code speak for itself aboutwhat it's doing.

Your members are implicitlyPublic. IfMainSub is meant to be called from the outside, or as a macro, then make itexplicitlyPublic, and then ifUserLogin is meant to only ever be called fromMainSub, then make itPrivate.

Your parameters are implicitly passedByRef. In many languages including VB.NET, parameters are passed by value by default; it's a good idea to specifyByRefexplicitly when youmean to pass them by reference.

The parameters are also implicitlyVariant, butPassword is meant to be aString andExitApplication is meant to be aBoolean; it's also a good idea to consistently specify a type, even when that type isVariant, andespecially when that type is NOTVariant: that way you'll allocate only the memory space youneed to allocate; no more, no less.

TheCall 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 explicitCall forMsgBox andInputBox function calls; there's no need to use an explicitCall syntax for your own functions either.

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

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

See, VBA allocates a memory space for that empty string, but not forvbNullString. 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.

TheInputBox result length-check is finein 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 theStrPtr string pointer value of the result: if that value is 0, the user cancelled theInputBox. If that value isn't 0, the usermeant 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 theExit Sub is heuristically unreachable code. It should be one conditional:

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

It's not clear whyConfirm is assigned to an empty string; it's aVariant/VbMsgBoxResult variable (implicitly declared as aVariant), so if anything it should be reset to 0, sinceVbMsgBoxResult is an enum type, and enum values are really justLong constants.

It looks particularly weird to seePassword = "" immediately followed byPasswordLength = 8 - seems you're giving different meanings toPasswordLength. At one point it stands for theexpected length, and at another it stands for theuser input length. This is confusing.

Remove it. All youreally need is this:

Const RequiredLength As Integer = 8

And then:

If Len(Password) <> RequiredLength Then

It reads much clearer, and removes the magic value8 from your code, too; the string message should also be using thatRequiredLength constant, so that if it ever needs to change to12 (itprobably 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.

• \$\begingroup\$Wow, this is just so incredible, that you took all of your time to type this! Thank you very much and a Merry Christmas! You should get some extra presents! I learned a lot and I am very thankful for clearing up a lot of my confusion. I will implement all of the above, or as much as I can and post the result!\$\endgroup\$

  EliasWick

  – EliasWick

  2016-12-12 06:34:08 +00:00

  CommentedDec 12, 2016 at 6:34

• vba
• excel