Some code feedback (C++)

[Karstein]

I'm currently dusting off the cobwebs with my C++ and making a real concerted effort to learn how to make some small games. With my most recent breakthroughs I was able to create a small Tic Tac Toe game in C++ & SFML. It works, and does what I originally set out to do, but was looking for some feedback on it from you knowledgeable folks as I know there are probably some gross inefficiencies with my code.

I applied things I have learned from various paths of learning, but there are also a number of cases where I basically tried something out to see if it worked, or tried to work out how to make something happen by rifling through documentation.

The code itself can be found here. Feel free to paste whatever elements you wish to comment on back on this topic. And here are some examples of it in action:

Pic1
Pic2
Pic3

The main questions I had really were:

• I didn't use any dynamic memory allocation (blah = new blah) in my entire code, though I see that it is popular to do from looking at other code samples. What is the benefit of this, and where should I have used it here?
• I was originally considering using a class (or classes) for the player pieces but went with functions instead. Should I have gone the class route?
• The overall logic that I used works (if the mouse click is within this area, mark that sector as claimed by whatever player's go it is), but I couldn't help but feel I was taking a cumbersome path somehow. Would you have done this differently, and if so, how?
• The logic to check the game state I also felt was cumbersome (the really... really extensive if/else statements). Could that have been cleaned up in any way?

Those are about all I can think of for now, don't hesitate to throw in any other observations you have. Many thanks in advance for helping me become a better code monkey!

 

[Merad]

Disclaimer that I haven't looked at your code in any significant detail, but I can address some of your questions.

I didn't use any dynamic memory allocation (blah = new blah) in my entire code, though I see that it is popular to do from looking at other code samples. What is the benefit of this, and where should I have used it here?

Probably pointless in a simple game like tic-tac-toe. You have one fixed board, with 9 spaces, and 2 images for the X and O. You know ahead of time exactly what you need, so you can easily set everything up on the stack.

Dynamic memory is necessary for things that are, well, dynamic. Any time you don't know the size of something when you're writing the code. Is the text file that you're loading going to be 10 bytes or 10000 bytes? Things like that.

I was originally considering using a class (or classes) for the player pieces but went with functions instead. Should I have gone the class route?

The only reason to do it in a program this simple would be as a learning exercise. OOP generally is more about trying to keep things better organized when you're dealing with complex programs.

As far as the last 2 questions - there are some ways I think you could improve your code organization, but I'm short on time... I'll post more tomorrow if no one else has by then.

Edit: 1 quick thing that caught my eye. You're loading all the textures every time you draw them. You can get away with it in something this simple but that would be a massive performance killer in a more complex game. Try to update your code to load them once before the game starts, then hold them in a variable and reuse them throughout the game.

 

[Karstein]

Thanks, most helpful!

 

[Pia]

There are some easily apparent duplicate pieces of code, like the if...else in the text output block (do "It's Player " + currentPlayer + "'s turn!" instead).

And the big duplicate if block checking for victory conditions - make it a function taking a parameter of which player's win to check for. Or a function which returns winning player's number or zero if neither won.

Also, it's ugly to have the graphic coordinates in a const table. They could all be calculated from square size in pixels and the amount of squares.

edit: oh, and you get the column clicked from ((mouseX - boardStartX) / squarePixels) and row from ((mouseY - boardStartY) / squarePixels), no need to loop and test vs individual squares.

 

[Karstein]

That all makes a lot of sense, thank you!

 

[eLiu]

• I didn't use any dynamic memory allocation (blah = new blah) in my entire code, though I see that it is popular to do from looking at other code samples. What is the benefit of this, and where should I have used it here?
• I was originally considering using a class (or classes) for the player pieces but went with functions instead. Should I have gone the class route?
• The overall logic that I used works (if the mouse click is within this area, mark that sector as claimed by whatever player's go it is), but I couldn't help but feel I was taking a cumbersome path somehow. Would you have done this differently, and if so, how?
• The logic to check the game state I also felt was cumbersome (the really... really extensive if/else statements). Could that have been cleaned up in any way?

!

Don't have time to look at code right now & also I'm lazy... but
dynamic mem: if, at compile-time, you know the sizes of every piece of storage you're going to use, then you can often avoid dynamic allocation. This would be pretty impossible if say your game had to handle an NxN board where the user specifies N at runtime. A few exceptions...
A) you're stack-allocating "enormous" amounts of space (=too big for program stack)
B) you need data persistence (since stack allocated variables are invalidated when their scope expires)... though you can design around this.

classes: this depends on what you were trying to learn from this exercise. If you want practice w/classes, then use them. If you do that, you might later consider a more complex problem, like supporting arbitrary numbers of players/types of pieces, different rule-sets, etc.

gui: no idea

game state: I'll throw out a few (similar) ideas:
A) Represent the board as a 3x3 array of integers. Say -1 for X, 0 for empty, 1 for O. Compute row, column, and diagonal sums. If any of these sum to -3 or 3, then X or O win, respectively. This has the advantage of being easily updated as moves are played out--you don't have to scan the whole board every time a move is made. Just update the appropriate sum(s) and check if anyone has won.
B) Each square needs 3 bits to determine it's state (X, O, empty). Represent the board using 3*9=27, or use 4 bits per square (total of 36, this is easier btw)... either way, whole board fits in 1 integer. If read the value of this integer, every board state corresponds to a unique integer. There a handful of possible values that can be wins (8 of them); check if your board's integer "value" matches any of these. Do it all with bitwise operations only so that only 1 if statement is required at the very end.
C) Represent each square w/a character representation, maybe '-' for empty and 'X' and 'O' for the players. Flatten the board into a character string 9 characters long. As in B), there are a finite number of 'win' states. You can specify these beforehand (or auto-gen them), then use regular-expressions to match your board state against known win-states.

 

[veri745]

I generally like to simplify the main loop in a program to a minimal set of descriptive calls. Otherwise giant program loops aren't very readable.
Something like:

while (!gameEnded)
{
   promptUser(currentPlayer);
   getPlayerInput(currentPlayer);
   drawWindow();
   gameEnded = checkVictoryConditions(currentPlayer);

   if(gameEnded) {
      displayGameOver();
   }

   updateCurrentPlayer(currentPlayer);

}

Like others have mentioned, you have lots of duplicate code, which would be reduced if you used more function calls.

 

[Karstein]

Awesome, lots to think about here. Really appreciate it!

 

[Cogman]

// Check to see if P1 has a legal line of 3 pieces.
				if ((sectorStates[0] == 1 && sectorStates[1] == 1 && sectorStates[2] == 1) ||
					(sectorStates[3] == 1 && sectorStates[4] == 1 && sectorStates[5] == 1) ||
					(sectorStates[6] == 1 && sectorStates[7] == 1 && sectorStates[8] == 1) ||
					(sectorStates[0] == 1 && sectorStates[3] == 1 && sectorStates[6] == 1) ||
					(sectorStates[1] == 1 && sectorStates[4] == 1 && sectorStates[7] == 1) ||
					(sectorStates[2] == 1 && sectorStates[5] == 1 && sectorStates[8] == 1) ||
					(sectorStates[0] == 1 && sectorStates[4] == 1 && sectorStates[8] == 1) ||
					(sectorStates[6] == 1 && sectorStates[4] == 1 && sectorStates[2] == 1))

This is yucky, confusing, and prone to errors. Try and avoid doing this.

Everytime you start to repeat a line of code, ask yourself "Could I do this in a function, loop, or use some math trick to compute this directly".

 

[cytg111]

I had a very hard time going from procedural "train-of-thought" to the object oriented, i remember my professor standing there explaining java, and all I could think is like "Yes, but why? I can do that allready, more efficient, why should i swallow your OO crap".

Truth is, it still creeps up on me some times, but fact is, "thinking oo" makes larger codebases more maintainable, human readable and so on. There is nothing wrong with NOT using classes if your scope does not warrant it. Right tool for the job period