Jump to content

  • Log In with Google      Sign In   
  • Create Account

#Actualrip-off

Posted 05 May 2013 - 06:59 AM

Congratulations on completing your game! Completing any game is no small task.

 

Here are some things I noticed when reviewing your code;

 

  • 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).

     

     

     


  • You should not use preprocessor defines for compile time constants. Use the language feature:

     

     

    const int SCREENWIDTH = 832;
    const int SCREENHEIGHT = 600;
    const int PADDLEYPOS = 550;
    const int MAXPADDLESPEED = 400; 

     

     


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

     

     

     


  • Consider using an enumeration for the brick types, rather than strings. A typo in a string literal will not be caught at compile time.

     

     

     


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

     

     

     


  • Boolean functions such as Game::isExiting can be written to return the expression, rather than write a full if/else statement:

     

     

     bool Game::isExiting(){
        return m_GameState == exiting;
    } 

     

     


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

     

     

    Instead of:

     

     case gamelost:
        lresult = m_inpMgr.lostEvents(m_Event);
        if (lresult == Input_Manager::nada); 
        else if (lresult == Input_Manager::mainMenu)
            m_GameState = title;
        break; 

     

    Consider omitting the "nada" branch:

     

     case gamelost:
        lresult = m_inpMgr.lostEvents(m_Event);
        if (lresult == Input_Manager::mainMenu)
            m_GameState = title;
        break; 

     

     


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

     

     

     if (gresult == Input_Manager::prev)
        m_GameState = title;
    else if (gresult == Input_Manager::paused)
        m_GameState = paused;
    else if (gresult == Input_Manager::idle) {
        if(m_Paddle.livesLeft() < 0) {
            m_GameState = gamelost;
        } else if(isLevelWon()) {
            m_GameState = gamewon;
        }
    } 

     

     


  • If you decouple the brick drawing from the brick type, you could probably have a vector of values rather than smart pointers.

     

     

     


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

     

     

     


  • 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);
    } 

     

     


  • Your coding convention is inconsistently applied. For example, Brick::alive and Ball::isBallMoving should start with "m_" to match the other members.

     

     

     


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

     

     

     


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

     

     

     


  • 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", 

     

     

     


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

     

     

     


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

     

     

     


: Editor eating my post formatting


#1rip-off

Posted 05 May 2013 - 06:57 AM

Congratulations on completing your game! Completing any game is no small task.

 

Here are some things I noticed when reviewing your code;

  • 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).

     

  • You should not use preprocessor defines for compile time constants. Use the language feature:

    const int SCREENWIDTH = 832;
    const int SCREENHEIGHT = 600;
    const int PADDLEYPOS = 550;
    const int MAXPADDLESPEED = 400;
     
    

     

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

     

  • Consider using an enumeration for the brick types, rather than strings. A typo in a string literal will not be caught at compile time.

     

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

     

  • Boolean functions such as Game::isExiting can be written to return the expression, rather than write a full if/else statement:

     
    bool Game::isExiting()
    {
        return m_GameState == exiting;
    }
     
    

     

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

     

    Instead of:

     
    case gamelost:
        lresult = m_inpMgr.lostEvents(m_Event);
        if (lresult == Input_Manager::nada);
     
        else if (lresult == Input_Manager::mainMenu)
            m_GameState = title;
        break;
     
    

     

    Consider omitting the "nada" branch:

     
    case gamelost:
        lresult = m_inpMgr.lostEvents(m_Event);
        if (lresult == Input_Manager::mainMenu)
            m_GameState = title;
        break;
     
    

     

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

     
    if (gresult == Input_Manager::prev)
        m_GameState = title;
    else if (gresult == Input_Manager::paused)
        m_GameState = paused;
    else if (gresult == Input_Manager::idle) {
        if(m_Paddle.livesLeft() < 0) {
            m_GameState = gamelost;
        } else if(isLevelWon()) {
            m_GameState = gamewon;
        }
    }
     
    

     

  • If you decouple the brick drawing from the brick type, you could probably have a vector of values rather than smart pointers.

     

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

     

  • 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);
    }
     
    

     

  • Your coding convention is inconsistently applied. For example, Brick::alive and Ball::isBallMoving should start with "m_" to match the other members.

     

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

     

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

     

  • 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", 

     

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

     

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

     


PARTNERS