1
\$\begingroup\$

I am hoping to make a quick decision tree using the if statement in excel vba. The point is to have a message with a prompt show up and then it will take the person through the prompts based on the answers. Can my current approach be improved upon in any way?

Sub Decision_tree()

    a = MsgBox("Want To Play A Game?", 3)

    If a = 6 Then
    MsgBox "Let the games begin!", 1

    ElseIf a = 7 Then
    MsgBox "Okay. Press ctrl K to start over."
    End

    Else
    MsgBox "Sad. Come back later. Press ctrl K to start."
    End
    End If

    a = MsgBox("are you a boy?", vbYesNo)

    If a = 6 Then '**
       b = MsgBox("does your names start before O?", vbYesNo)
       If b < 7 Then d = MsgBox("do you live in logan?", vbYesNo)
            ElseIf b = 7 Then
                   f = MsgBox("Is your name 4 letters?", vbYesNo)
                     If d = 6 Then
                       h = MsgBox("Did you serve your mission with Josh?", vbYesNo)
                       ElseIf f = 6 Then
                        MsgBox "Your name is Ryan"
                         ElseIf f = 7 Then
                        MsgBox "your name is Trevon"
                            ElseIf d = 7 Then
                                   j = MsgBox("Did you serve a mission with Josh?")
                                   ElseIf h = 6 Then
                                   MsgBox "Your name is Cade"
                                    ElseIf h = 7 Then
                                           K = MsgBox("are you a DJ?", vbYesNo)
                                             ElseIf j = 6 Then
                                             MsgBox "your name is Houston"
                                              ElseIf j = 7 Then
                                              MsgBox "your name is Cameron"
                                                ElseIf K = 6 Then
                                                MsgBox "your name is Jack with a j"
                                                 ElseIf K = 7 Then
                                                  MsgBox "your name is burton"                        

    ElseIf a = 7 Then
       c = MsgBox("does your name start Before N?", vbYesNo)
        ElseIf c = 6 Then
            e = MsgBox("does your name have a K in it?", vbYesNo)
            ElseIf c = 7 Then
                   g = MsgBox("do you own a jeep?", vbYesNo)
                   ElseIf e = 6 Then
                      i = MsgBox("Are you from Idaho?", vbYesNo)
                       ElseIf g = 6 Then
                        MsgBox "your name is olivia"
                        ElseIf g = 7 Then
                        MsgBox "your name is tiffany"
                            ElseIf i = 6 Then
                            MsgBox "Your name is Kasey"
                             ElseIf i = 7 Then
                              MsgBox "Your name is Mckenna"

       End If
    End If

End Sub
\$\endgroup\$
  • \$\begingroup\$ I've edited your question a bit because before it came across like you hadn't worked out how to implement the decision tree and your code wasn't working yet, which would have been out of scope for the site (and put off potential reviewers). Also some of your code wasn't formatted correctly so I hope I've fixed that for you, but does the indentation look the same as it does in Excel? Is that how you'd expect it to be? If not, then just paste your code straight from Excel editor and put triple back ticks ``` above and below the code, and it should appear indented exactly the same as the original \$\endgroup\$ – Greedo Apr 10 at 8:36
  • 1
    \$\begingroup\$ The inconsistent indenting makes this really difficult to read. \$\endgroup\$ – FreeMan Apr 10 at 10:46
  • 1
    \$\begingroup\$ I'd have to say that code doesn't work because it won't even compile. At the very minimum, it's missing an 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\$ – FreeMan Apr 10 at 10:53
  • 1
    \$\begingroup\$ @FreeMan There is no missing End If because one of the If statements is a single-line If statement. \$\endgroup\$ – M.Doerner Apr 10 at 11:43
  • 1
    \$\begingroup\$ @M.Doerner That indentation really is misleading then! Maybe op could use Rubberduck's indenter to get the code looking nice, there's an interactive online example here - just paste the code in the box to have it indented. Or install the addin so you can indent from within Excel (and do a lot of other stuff too) \$\endgroup\$ – Greedo Apr 10 at 12:00
8
\$\begingroup\$

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. ElseIfs 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.

  1. 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.
  2. 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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wow thank you @Ivenbach this was super informational. Especially for someone like me who is an newbie to vba. I will start doing this. Thanks for the help! \$\endgroup\$ – Josh Bullough Apr 11 at 3:44
  • \$\begingroup\$ Uh... I just edited 2 minor typos. It is @M.Doerner whom you should be thanking. He’s the one that wrote that article, not me. \$\endgroup\$ – IvenBach Apr 11 at 4:31
  • \$\begingroup\$ Thanks @JoshBullough that you appreciate my answer that much. If you find it that helpful, you might want to mark it as the answer. Alternatively, you could wait a few more days to see whether a better answer will be posted. \$\endgroup\$ – M.Doerner Apr 11 at 10:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.