VBA Excel Game - Testing

Question

My brother bet me that I couldn't make a game in Excel. The code is all over the place and buggy in parts. But I would appreciate it if anyone would give this game a go.

The music and the code has all been made by me, but the artwork was found online and I can't claim any of it is mine (although a few bits were photoshopped to make it work for me).

There's still a few bits of the game that don't work 100% but it shouldn't crash on you.

Full code

There's an installer or the zip file - it's probably best to just use the zip file.

Sub dogame()
Application.Visible = False
loadedgame = False
Do
 fminitScreen.Show
 playgame
 If loadedgame = False Then
 loadplayer
 fmNameHim.Show
 fmChooseWeapon.Show
 If Worksheets("options").range("B3").value = True Then
 fmrules.Show
 End If
 fminside1.Show
 ElseIf loadedgame = True Then
 Dim obj As Object
 Dim obj1 As Object
 Load fminside1
 Load fmoutside
 Load fmupstairs
 For Each obj In UserForms
 If obj.Name = player.formname Then
 loadedgame = False
 obj.Character.left = player.left
 obj.Character.top = player.top
 obj.Show
 Exit For
 Else
 If obj.Name = "fmMusic" Then
 Else
 Debug.Print "unloaded form" & obj.Name
 Unload obj
 End If
 End If
 Next obj
 End If
 fmranking.Show
 Set player = Nothing
Loop
End Sub

• user28366: nice name for a dog : ame ;)

Answer 1

Procedure names should start with a verb that says what the procedure does.

Sub dogame()

That doesn't say much; "do" is as general as it gets. Also, by convention, procedure names should be PascalCase, so Sub DoGame() would follow that.

You're not showing where loadedgame is declared, but if it's declared, I suspect it's a Boolean variable, at module level. If dogame is the only procedure that uses this variable, then loadedgame should be scoped to that procedure, and declared as close as possible to its usage:

Sub RunGame()
 Dim loadedGame As Boolean

Boolean variables get initialized with a False value, so if loadedgame is scoped at procedure level, this line isn't needed:

loadedgame = False

It's a very, very good idea to always declare all variables you're using. One way of forcing yourself to do that, is to specify Option Explicit at the top of every code module you're writing - whether it's a form's code-behind, a class module, or a standard module: with that option specified, VBA will refuse to compile your project if an undeclared identifier is used anywhere.

---

Your indentation is somewhat broken in the inner For loop, and I strongly recommend indenting the entire procedure's body too:

Sub RunGame()
 '<~ indented
End Sub

The If Worksheets("options").Range("B3").Value = True Then condition doesn't need to be indented - it's at the same level as the previous fmChooseWeapon.Show line.

---

VBA module members are Public by default, but in other languages it's Private by default; it's always best to be explicit about access modifiers:

Public Sub RunGame()

I think there's way too much vertical whitespace. There shouldn't be 2 empty lines in a row.

---

Boolean values can be used as a condition's expression - If [Boolean expression] Then can also be If [Boolean value] Then, so this:

If loadedgame = False Then
 'block A
ElseIf loadedgame = True Then
 'block B
End If

Is better written like this:

If Not loadedgame Then
 'block A
Else
 'block B
End If

...and best written like that:

If loadedgame Then
 'block B
Else
 'block A
End If

Positive conditions are always easier to mentally process; don't be afraid to reverse the If and Else blocks to reverse a negative condition! Also, a Boolean can only ever be True or False, so there's no need to evaluate the False condition if you've already checked the True condition - the ElseIf block can become an Else block without altering the logic.

---

This part is weird:

If obj.Name = "fmMusic" Then
Else
 Debug.Print "unloaded form" & obj.Name
 Unload obj
End If

If there's nothing to execute in the If block, only in the Else part, then you need to reverse your condition:

If obj.Name <> "fmMusic" Then
 Unload obj
 Debug.Print "unloaded form" & obj.Name
End If

Notice I've moved the Debug.Print call after the Unload instruction - this way the immediate window will not say "unloaded form xyz" before the form is actually unloaded. Alternatively, you could edit the message to say "unloading form " & obj.Name & "..." - the idea is to avoid telling confusing lies if anything goes wrong with that Unload call. That said, obj is a very bad name to use - why not call it form or window?

---

The procedure is quite long - I'd consider extracting smaller private procedures out of it, perhaps starting with the two branches in the If loadedgame condition. A procedure should do one thing (and do it well), so as to have as few reasons to change as possible. Having more specialized procedures helps making your code more manageable, too.

Lastly, I suspect this is all written in a standard code module; I would encapsulate the game logic inside a class module, and have the dogame procedure instantiate an object of that type and call some Run method. The entire dogame procedure/macro could then possibly look like this:

Public Sub RunGame()
 On Error GoTo ErrHandler
 Dim game As New GameLogic
 game.Run
CleanExit:
 Exit Sub
ErrHandler:
 Debug.Print "An error has occurred: " Err.Description
 Resume CleanExit
End Sub

• user28366: ++ usually Bool/Boolean should start with IsSomething :) Microsoft doesn't always follow that principle but it's good if we do it makes code analysis a lot easier :)
• Lewis Morris: Download it and play it there are tons on classes and more. I want to learn more and i appreciate all your comments and don't take offence to any of it and will only learn and grow. I need experience from people you like to guide me into the future.

Answer 2

I'm going to go down through the code line by line. I may be harsh at times, but remember, I say these things so that you might become a better programmer.

---

Sub dogame()

The sub name is a bit hard to read. I personally like the .Net capitalization standards where methods are PascalCased. Isn't DoGame easier to read? Also, get into the habit of explicitly declaring scope.

Application.Visible = False

I like that you're hiding the Excel instance from the user, but what happens if an error occurs? Without an error handler to regain control and make Excel visible again, you'll end up with an instance of Excel that you can only kill via the task manager.

loadedgame = False

Remember those naming conventions I mentioned earlier? Variables should be camelCased. So, loadedGame. However, I think it's easier to read as isGameLoaded.

The next thing I want you to do is start using Option Explicit in all of your vba code. It forces you to declare all of your variables. This will turn typos into compile time errors. It's wonderful. Trust me, just do it.

Lastly, booleans default to false, so if you declare the variable using a Dim statement, there is no need to explicitly set it to false.

---

To recap so far:

Option Explicit
Public Sub 
 On Error GoTo ErrHandler
 Dim isGameLoaded As Boolean
 Application.Visible = False
 '...
 Exit Sub
ErrHandler:
 Application.Visible = True
End Sub

---

Which brings us to the Do loop. Friend, that is just awful. You've intentionally created an infinite loop. Once the DoGame is called, it never exits. Never. The only way I can figure to kill this is to open the task manager and kill Excel.

What you should do is have a While loop that exits when the game is over. For example, consider this code where GetGameState is a function that returns a boolean.

Dim gameOver As Boolean
While Not gameOver
 gameOver = GetGameState
End While 

---

fminitScreen.Show
playgame

Again with the capitalization. This is hard to read. Not much else to say other than if you must use Hungarian notation for your user forms, then you should at least use Microsoft's recommended notation. fminitScreen should be frmInitScreen. But really InitializationScreen or StartScreen would be much better names. (I was going to provide a link, but it seems that MS has scrubbed that document from the face of the Earth...)

---

Skipping ahead a bit, there's this.

If Worksheets("options").range("B3").value = True Then

First, we have no clue what this value is, other than it's some option of some kind. I recommend creating a well named function to retrieve this value.

Secondly, the statement can be simplified by omitting = True. This is essentially what happens right now.

 Worksheets("options").range("B3").value = True
' get value from worksheet 
True = True
' is true equal to true?
True
' enter if statement 

That's why you can write this like this.

If Worksheets("options").range("B3").value Then

Same thing here. Simplify it.

 ElseIf loadedgame = True Then

ElseIf isGameLoaded Then

---

This is bad. Could these names be more generic?

 Dim obj As Object
 Dim obj1 As Object

What really makes this terrible is the type doesn't even give us a hint because you've declare them as generic objects. You could literally stuff any class instance you wanted to into those variables.

It looks like you're using obj to iterate the UserForms collection

 For Each obj In UserForms

So why not declare it as a UserForm???

Dim obj As UserForm

Or better yet

Dim frm As UserForm