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 bePascalCase, soSub DoGame() would follow that.

You're not showing whereloadedgame is declared, but if it's declared, I suspect it's aBoolean variable, at module level. Ifdogame is the only procedure that uses this variable, thenloadedgame should bescoped to that procedure, and declared as close as possible to its usage:

Sub RunGame()    Dim loadedGame As Boolean

Boolean variables get initialized with aFalse value, so ifloadedgame 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 specifyOption 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 innerFor loop, and I strongly recommend indenting the entire procedure's body too:

Sub RunGame()    '<~ indentedEnd Sub

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

VBA module members arePublic by default, but in other languages it'sPrivate 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 beIf [Boolean value] Then, so this:

If loadedgame = False Then    'block AElseIf loadedgame = True Then    'block BEnd If

Is better written like this:

If Not loadedgame Then    'block AElse    'block BEnd If

...and best written like that:

If loadedgame Then    'block BElse    'block AEnd If

Positive conditions are always easier to mentally process; don't be afraid to reverse theIf andElse blocks to reverse a negative condition! Also, aBoolean can only ever beTrue orFalse, so there's no need to evaluate theFalse condition if you've already checked theTrue condition - theElseIf block can become anElse block without altering the logic.

This part is weird:

If obj.Name = "fmMusic" ThenElse    Debug.Print "unloaded form" & obj.Name    Unload objEnd If

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

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

Notice I've moved theDebug.Print callafter theUnload instruction - this way theimmediate 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 thatUnload call. That said,obj is a very bad name to use - why not call itform orwindow?

The procedure is quite long - I'd consider extracting smaller private procedures out of it, perhaps starting with the two branches in theIf 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 thedogame procedure instantiate an object of that type and call someRun method. The entiredogame procedure/macro could then possibly look like this:

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

• \$\begingroup\$++ usuallyBool/Boolean should start withIsSomething :) Microsoft doesn't always follow that principle but it's good if we do it makes code analysis a lot easier :)\$\endgroup\$

  user28366

  – user28366

  2014-10-24 07:07:28 +00:00

  CommentedOct 24, 2014 at 7:07
• \$\begingroup\$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.\$\endgroup\$

  Lewis Morris

  – Lewis Morris

  2014-10-25 21:28:02 +00:00

  CommentedOct 25, 2014 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'tDoGame easier to read? Also, get into the habit of explicitly declaringscope.

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

The next thing I want you to do is start usingOption Explicit in all of yourvba 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 aDim statement, there is no need to explicitly set it to false.

To recap so far:

Option ExplicitPublic Sub     On Error GoTo ErrHandler    Dim isGameLoaded As Boolean    Application.Visible = False    '...    Exit SubErrHandler:    Application.Visible = TrueEnd Sub

Which brings us to theDo loop. Friend, that is just awful. You've intentionally created an infinite loop. Once theDoGame 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 aWhile loop that exits when the game is over. For example, consider this code whereGetGameState is a function that returns a boolean.

Dim gameOver As BooleanWhile Not gameOver    gameOver = GetGameStateEnd While

fminitScreen.Showplaygame

Again with the capitalization. This is hard to read. Not much else to say other than if youmust use Hungarian notation for your user forms, then you should at least use Microsoft's recommended notation.fminitScreen should befrmInitScreen. ButreallyInitializationScreen orStartScreen 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 valueis, other than it'ssome 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 asgeneric objects. You could literally stuff any class instance you wanted to into those variables.

It looks like you're usingobj to iterate theUserForms collection

   For Each obj In UserForms

So why not declare it as aUserForm???

Dim obj As UserForm

Or better yet

Dim frm As UserForm

• game
• vba
• excel