Let me start with a few quick things to enhance readability and then give some pointers how one could make this more maintainable, if one wanted to.
Magic Numbers
One thing that makes your code hard to follow is that you use magic numbers instead of named constants at multiple places. You already use vbYesNo
for the MsgBox options most of the time. I wonder why you use 3
instead of vbYesNoCancel
in the first message box. Moreover, the code would be more intuitive, if you replaced 6
by vbYes
and 7
by vbNo
. Whenever you want to use an explicit number in code, ask yourself whether there is a constant for that already and, if not, define a constant of your own and give it a semantic name.
Indentation
As @Greedo has pointed out in the comments, the strange indentation might be an artifact of how you have posted the code. However, as is, the indentation makes the code unreadable. ElseIf
s should line up with the corresponding If
statements and their content should be indented one level deeper. Then, you know which level of the tree you are in while reading the code.
Reuse of Variables
It is generally confusing in code, if you reuse a local variable. There is no benefit from reusing the variable a
here. Still, the reader of the code has to know that it has been reused for an entirely different question in the second part of the procedure.
Naming
Code should always strive to be self-documenting. The most important part in this is good naming of variables and procedures. They should always have a name indicating what they are/do; variables, properties and functions should be nouns and procedures should be verbs. Here, expressiveness trumps shortness: there is no problem with having a long name if you cannot express the meaning in a short name. If this leads to a very long name, it might be a smell that you should split something up.
In you specific case, all your answers are single letter variables without any meaning, which is not ideal. E.g. a better name for e
, or rather e = vbYes
, would be nameStartsWithK
.
Note that applying semantic naming would have avoided the reuse of the variable a
automatically since there is no sensible common name for the two uses.
Declaring Variables
Do yourself a favor and always declare you local variables, e.g. using Dim a As Long
right before using a
the first time. This documents the variables type and subsequent assignment with an incompatible type will result in an error instead of unexpected behaviour. (VBA does a lot of helpful implicit conversions, though.)
Option Explicit
Please, always use Option Explicit
on top of your component. You can let the editor add it automatically for new components by ticking Require Variable Declaration under Tools->Options->Editor. This will force you to declare all your variables. A very helpful side-effect is that it will catch typos in uses of your variables. E.g. is you try to reference nameStartsWithK
but type nameStartWithK
, it will complain that nameStartWithK
is not declared. Without Option Explicit
, you get a new variable of type Variant
instead, which is really not what you want here.
Use of End
The End
statement is something that usually should never be used. In general, it is unexpected behaviour that a called procedure terminates the entire program. Instead, the procedure should probably use Exit Sub
in order to return execution to its caller.
Use of Single-Line If Statements
Single-line If statements like If b < 7 Then d = MsgBox("do you live in logan?", vbYesNo)
are generlly a bit confusing because there is no End If
one starts to expect out of habit. However, this is amplified tremendously without proper indentation since it is not immediate clear for the reader what the next ElseIf
refers to.
Select Case Statement
For your first If
block, a Select Case statement would probably be semantically more appropriate.
Select a
Case vbYes
MsgBox "Let the games begin!"
Case vbNo
MsgBox "Okay. Press ctrl K to start over."
Case Else
MsgBox "Sad. Come back later. Press ctrl K to start."
End Select
Note that I have removed the option 1
in the first case, which is vbOKCancel
. More on this, next.
User Experience
Your first query to the user whether he wants to play a game has two somewhat strange features.
- When you confirm that the game starts, you do this in a message box with option
vbOKCancel
but never react to the result. As the user, I would expect that clicking the Cancel will abort the game. If the effect of Cancel
is the same as OK
, do not display a Cancel
button.
- It is usually percieved as rather annoying, if a dialog pops up in response to cancelling. This is exactly what happens in your first query to the user.
Using Indirection to Improve Maintainability
When speaking about maintainability and flexibility one has to ask what you might want to change in the future and what is a hindering proper automated testing. In the case of this simple game, the latter is probably not a real concern, but for parts of larger applications, it is.
The main point of possible change I see here, is the presentation to the user. Not entirely by coincidence, this is also what would hinder any automated testing since blocking dialogs are a pain for that.
This is a bit more advanced, but what you could do to abstract away the use of the message box is to add a new class YesNoUserInteraction
with a single function
Public Function UserAgrees(ByVal questionText As String) As Boolean
UserAgrees = (MsgBox(questionText , vbYesNo) = vbYes)
End Function
The you new it up and save it in a variable Dim userInteraction As YesNoUserInteraction
in your code Set userInteraction = New YesNoUserInteraction
and use that instead of the message box.
Going one step further, you can split out the part newing up the object and pass it as a parameter to the procedure.
Public Sub Decision_tree()
Dim userInteraction As YesNoUserInteraction
Set userInteraction = New YesNoUserInteraction
DecisionTreeImpl userInteraction
End Sub
Private Sub DecisionTreeImpl(ByVal userInteraction As YesNoUserInteraction)
...
End Sub
This has the benefit that another class could implement YesNoUserInteraction
and that could be provided as the user interaction to the implementation instead. This way, you can change the presentation entirely, e.g. using a custom user form, or automate the answers for unit testing.
The way it is, the intent to allow other implementations is not really clear though. We can go one step further and introduce a dedicated interface class IYesNoUserInteraction
for the interaction.
Public Function UserAgrees(ByVal questionText As String) As Boolean
End Function
Then, we can turn YesNorUserInteraction
into YesNoMsgBoxInteraction
and implement the interface.
Implements IYesNoUserInteraction
Private Function IYesNoUserInteraction_UserAgrees(ByVal questionText As String) As Boolean
UserAgrees = (MsgBox(questionText , vbYesNo) = vbYes)
End Function
To get the right names and signatures for the members to implement, you can use the two drop downs at the top of the code pane.
Finally, we can use it in the setup code.
Public Sub Decision_tree()
Dim userInteraction As IYesNoUserInteraction
Set userInteraction = New YesNoMsgBoxInteraction
DecisionTreeImpl userInteraction
End Sub
Private Sub DecisionTreeImpl(ByVal userInteraction As IYesNoUserInteraction)
...
End Sub
This makes it clear that we can provide anything to the procedure that honors the contract to take a question and return an answer in the form of a Boolean with True = Yes
.
Obviously, this does not deal with the first question to the user, since you want to allow cancellation. You could do that in a similar way as presented here, but not with a simple Boolean.
Separation of Responsibilities
The last observation above points to a further thing that could be improved in your code. Currently, it violates the so-called Single Responsibility Principle. In essence, it states that any unit of computation, e.g. a procedure, should be responsible for one and only one thing. Yours actually is for two: asking the user whether he wants to play at all and playing the game.
What you could do instead is to extract a function and a procedure. The function could ask whether the user wants to play and return True
if he wants to play and False
, otherwise. Then, the procedure actually playing the game is only called if the user wanted to play.
Since the procedure is not large, this does not seem to be much of a problem that it might change for entirely different reasons: changing how a user is asked and playing the game differently. However, if you ever want to write something more complicated, you should get into the habit of splitting things up into small units with a defined purpose. That makes reading the code and figuring out what it actually does later on much easier.
End If
at the end. Does it actually do what you want it to do, and you're looking to make it do so in a better, more readable, more logical way; or does it not work and you're looking to figure out why? \$\endgroup\$End If
because one of the If statements is a single-line If statement. \$\endgroup\$