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

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

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

StackExchange Code Review Q#67730, answer score: 4