# VBA Excel Game - Testing

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

Do

fminitScreen.Show
playgame

fmNameHim.Show
fmChooseWeapon.Show

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

fmrules.Show

End If

fminside1.Show

Dim obj As Object
Dim obj1 As Object

For Each obj In UserForms

If obj.Name = player.formname Then

obj.Character.left = player.left
obj.Character.top = player.top
obj.Show
Exit For
Else
If obj.Name = "fmMusic" Then

Else

End If

End If

Next obj

End If

fmranking.Show
Set player = Nothing

Loop

End Sub

• nice name for a dog : ame ;) – user28366 Oct 24 '14 at 7:05

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()



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
'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

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

• ++ 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 :) – user28366 Oct 24 '14 at 7:07
• 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. – Lewis Morris Oct 25 '14 at 21:28

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

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