@rip-off: Wow. I was hoping for somebody to dissect my code like that. I really appreciate the time you took to look at my game. Thanks a lot.
By placing the projects own header files in stdafx.h, you are causing the entire game to be recompiled when any header changes. This might be manageable in smaller projects, but it will not scale as your project grows. As the automatically generated comments in stdafx.h state, the file should generally be used for external headers (e.g. SFML). There are "internal" headers that it may make sense to include here, such as commonly included utility headers (random.h might be a good candidate).
I'll remember that for my next C++ project. Now that I think about it with what I have done, I have defeated the whole purpose of using pre-compiled headers. I feel like an idiot 
.
You should not use preprocessor defines for compile time constants. Use the language feature
I have made this change, but can you tell me the downside of using preprocessor defines and what are the benefits of using compile time constants, as for this program I don't apparently see the benefit of using constants. To me the only difference I see is the program would have to allocate memory for the constants, and no memory would be allocated of the "define".
Your game class has a a number helper functions, for example setTextures(). These should be "private", not "public". Likewise, try to design your program so you do not have public fields. The bricks and window members should be private. The event member can probably be a local variable to your event handling code.
The function setTextures() and the member m_Window could have been made private, but I'm passing the brick object to another class which would then need to call some of its functions, if I set it to private the class to which the brick was passed to will not be able to call its functions and that is why I have made this public. I see how the event member should have been part of the event handling class. I'll be sure to adjust this accordingly for my next game. Thanks
You seem to have a lot of static variables. I don't think you really need any of them. It should be simple to convert them to members of the Game class. There may be better solutions.
Yes I could have made them members of the game class, but I didn't want my game class to be littered with a hundred members. So, instead I decided to make these variables local to the functions that needed them and made them static so they persist even when the function ends. These variables are not needed anywhere except the functions of they are a part of. In a much bigger project what would be a disadvantage or danger of using these static variables?
Boolean functions such as Game::isExiting can be written to return the expression, rather than write a full if/else statement:
Thanks I've made this change. This was really basic I should have known that m_GameState == exiting; will return either a true or false depending on the value in m_GameState.
If statements ending in a semicolon is highly unusual and normally sets alarm bells ringing for most programmers. Consider at least moving the semicolon to the next line and adding a comment saying "intentionally blank" or something. Better still is to avoid writing if conditions that require no logic.
Noted. But the reason I have it there was that it was possible for the lresult variable to get a value of "nada". So I explicitly included it there and told the compiler to not do anything if that happens.
Some parts of your Game class are complicated by trying to handle each case in one large if/else block. Moving sub-conditions into sub-blocks makes it much easier to read and understand:
I see, yeah that if/else block that you highlighted is pretty ugly. I've made the change like you have suggested looks a lot more readable now.
If you decouple the brick drawing from the brick type, you could probably have a vector of values rather than smart pointers.
There is a reason why I did that. My brick object has a property called "alive". I only want to draw bricks that are marked alive = true. Whenever a ball hits a brick that is breakable the bricks value is changed to alive = false. So during my for loop I have to check each brick's status. Now if you have a better solution to this problem please let me know how I can decouple the brick drawing from the brick type.
Your Game_Object class has a virtual member function, it's destructor should also be virtual. Alternatively, consider removing the virtual initialise function, instead require all game objects to be correctly initialised during construction. The base class should require the texture be passed through it's constructor, it should not be necessary to then allow clients to change the texture afterwards.
What benefit would I have for making the destructor virtual? There is nothing that I am allocating on the heap for any of the derived classes of the Game_Object class. The paddle and ball are allocated on the stack so destruction of those should be done when the stack gets destroyed. Yes I am allocating the bricks on the heap, BUT i am using smart pointers so that should take care of the destruction automatically for me right? There is no member variable in any of the derived classes that gets allocated to the heap, only the object itself (in the brick's case) gets allocated to the heap. Am I missing something here? The initialize function is different for the derived classes, it basically initializes the position back to the center of the screen for the ball and the paddle. This function gets called each time the player misses the ball or hits the red brick which also counts as a life lost. I need to call this function several times in the game, if I moved the contents of this function the the constructor then I wouldn't be able to initialize the position of the paddle and the ball each time the player misses the ball.
I'll remove the setTexture() function from the Game_Object class and actually set the texture during construction.
Sometimes code can be easier to read if you pull a common expression into a local variable. Compare:
// Original
sf::Vector2f Game_Object::getCenter(){
return sf::Vector2f(m_Sprite.getGlobalBounds().left + m_Sprite.getGlobalBounds().width / 2, m_Sprite.getGlobalBounds().top + m_Sprite.getGlobalBounds().height / 2);
}
// Local variable
sf::Vector2f Game_Object::getCenter(){
sf::FloatRect bounds = m_Sprite.getGlobalBounds();
return sf::Vector2f(bounds.left + bounds.width / 2, bounds.top + bounds.height / 2);
}
Wow, that looks really clean and is easily readable, I'll be sure to keep this in mind. I have done similar sort of this ugly stuff in several places, and it makes it hard to understand when I go back and look at it.
Your coding convention is inconsistently applied. For example, Brick::alive and Ball::isBallMoving should start with "m_" to match the other members.
Thanks for pointing that out, I have made the change.
The logic in Ball::handleCollisions is very complex and appears to contain much duplicate code. Consider splitting this into multiple functions, one of which determines if any collision occurred, another which calculates which way to bounce, and another one which handles the red/blue brick distinctions.
This function is probably the ugliest part of my game. I had a lot of problems with how I should go about handling the collision with the bricks, and my limited math knowledge only made it worse. The majority of the if else blocks are determining the various directions of the ball hitting the brick and reacting differently to where the ball hits the brick. I just couldn't come up with a better solution due to my limited math knowledge. You're right it could have been easier on the eyes if I broke it down into different functions but this was one of the last things I had to do to finish my game and I just hacked together this code, as I was losing motivation on this project fast (I wanted to move on to another project), but I forced myself to finish this game.
I spent at least three or four days on just this function alone. I am considering using Box2D to handle the physics and collision for my next projects, or I'll have to take the time and learn the math. Coming into gamedev I had no idea the amount of stuff that goes into making even the smallest of games, there is so much more that I still need to learn that it sometimes feels too overwhelming, which will probably make me go the easier route and use Box2D .
Your input manager is tightly coupled to the specifics of the game states. In one instance, it requires detailed knowledge of where the buttons are and their dimensions. This is exacerbated by the fact that magic numbers are used here, rather than named constants.
I'll probably make a button class for the UI stuff in later projects, where each button knows its position and will be able to tell the game when it was clicked instead of using magic numbers. What would be a better way to handle the input?
Your level loading code computes the positions for each brick as it loads individual bricks. This could probably be moved into a separate loop that is run after the individual bricks are instantiated. In fact, your current code contains a bug because the "count" variable in initialised at the start of the function, but it gets incremented many times as the two loops run. "yPos", and to a lesser extend "xPos",
The loop is separate. It appears to be inside the previous loop because the indentation is messed up. The count variable is also fine. It is used in only the second loop, the first loop ends before the second one starts. I have a const variable called BRICKROW = 13, which means that I can only have 13 bricks in one row, so after the 13th brick has been set in a row the yPos should be incremented with the yOffset so new row could be made and the xPos is initialized back to 0.0f. I do this by using that count variable that I have declared before the loop starts, when each brick is set, I increment count by 1, when count reaches 13, then the if block (if (count % BRICKROW == 0)) kicks in and the xpos and ypos are adjusted for the new row. The next time the new row will be made when count reaches 26 because the remainder will then be 0 again when dividing 26 by 13, after that the new row will be made when the count reaches 39 so on and so forth. It should not cause a bug.
I could have done this a little differently and reset the count back to 0 when the new row was made but then I would have had to change the if block to ( if (count == BRICKROW)). Both of these solutions do the same thing. Now unless I am totally missing something please let me know what it is I need to be doing.
Your level loading code does not handle errors very well. Ideally, your code should be able to report to the user if the file is missing, unreadable or corrupt. At the moment, your code prints a log message in the first case, but it appears that the program would still try to run without any bricks. If the file itself is corrupt, then you'll get very strange results.
You're right, I am aware of this. I should be throwing an exception here and then handling it accordingly, but as I have stated above I was in a hurry to just finish this game, and this part I wrote at the end so I was too lazy to do all that. I promise this won't happen in my future projects I swear 
.
There is no need to declare and define empty destructors, unless you wish to mark them as "virtual", or perhaps to show that the class obeys the rule of three but for some reason the destructor ends up being empty (in that case I would also consider adding a comment to this effect). Speaking of the rule of three, in larger projects consider marking classes as noncopyable where they cannot or should not have value semantics.
I wasn't the one that put them there. Visual C++ did that for me automatically, when it created the class for me. I had no intention of putting any code inside the destructors, but I decided to leave them there as I thought they aren't doing anything so no harm. I'll remove them if they get put in there automatically for all my future projects if I don't intend to use them. I had briefly read about the rule of three when I was learning C++ but I had totally forgot what it was, so thanks for reminding me, I'll keep the rule in mind next time.
For my next project I have decided to use C# and XNA, that should remove most of the C++ related problems like including headers etc. I'm already enjoying C# a lot and XNA makes a lot of the stuff real easy. Thanks for reviewing my code, I really appreciate it.
