Simple chain reaction game

Question

Not long ago I tried to get a game developer job (casual games). There was a task to write a simple game like this one. I was supposed to use an existing game engine (given in the task) and only create a game logic. After writing the solution I got the answer that my code has a bad style and not accepted for the review.

BubbleGame.h

#ifndef __BUBBLEGAME_H__
#define __BUBBLEGAME_H__

using namespace std;
using namespace Render;


class Bubble
{
    friend class BubbleGame;    

    typedef void (*pfnBubbleUpdate)(Bubble *bubble, float elapsedTime);

private:

    FPoint m_velocity;

    FPoint m_position;

    Color m_color;

    float m_workTime;

    float m_radius;

    bool m_isAlive;

    bool m_isExploded;

    pfnBubbleUpdate m_pfnBubbleUpdate;

    ParticleEffect *m_particleTrail;

protected:

    void PreventOutbound(float elapsedTime);

public:

    static FRect Bound;

    static float BornTime;

    static float LiveTime;

    static float FadeTime;

    static float StartRadius;

    static float DiffRadius;

    static EffectsContainer Effects;


    static void FinishAllEffects();

    static bool IsIntersect(const Bubble *left, const Bubble *right);


    static void FreeBubbleUpdate(Bubble *bubble, float elapsedTime);

    static void BornBubbleUpdate(Bubble *bubble, float elapsedTime);

    static void LiveBubbleUpdate(Bubble *bubble, float elapsedTime);

    static void FadeBubbleUpdate(Bubble *bubble, float elapsedTime);


    void Update(float elapsedTime);

    void Reset();

    bool IsAlive() const { return m_isAlive; }

    bool IsExploded() const { return m_isExploded; }

    void Explode();
};


class BubbleGame
{
private:

    BubbleGame() { /* Never used */ };

    BubbleGame(const BubbleGame& bubbleGame) { /* Never used */ };

    vector<Bubble> m_bubbles;

    vector<int> m_explodedIndices;

    vector<int> m_freeIndicies;

    Texture *m_bubbleTexture;

    bool m_isActive;

    int m_pointCount;

protected:

    void Initialize();

    void CheckInteractions(int explodedBubbleIndex);

    void CheckInteractions();

    void RemoveDeadBubblesIndices();

public:

    BubbleGame(int pointCount, float bornTime, float liveTime, const FRect *bound);

    ~BubbleGame();

    void Trigger(const IPoint& position);

    void Update(float elapsedTime);

    void Draw();

};

#endif // __BUBBLEGAME_H__

BubbleGame.cpp

#include "stdafx.h"
#include "BubbleGame.h"


FRect Bubble::Bound;

float Bubble::BornTime;

float Bubble::LiveTime;

float Bubble::FadeTime;

float Bubble::StartRadius;

float Bubble::DiffRadius;

EffectsContainer Bubble::Effects;


void Bubble::FinishAllEffects()
{
    Bubble::Effects.Finish();
}

bool Bubble::IsIntersect(const Bubble *left, const Bubble *right)
{
    float xDistance = abs(left->m_position.x - right->m_position.x);
    float yDistance = abs(left->m_position.y - right->m_position.y);
    float distance = sqrt(xDistance * xDistance + yDistance * yDistance);
    float explodeDistance = left->m_radius + right->m_radius;

    if (distance <= explodeDistance)
    {
        return true;
    }
    else
    {
        return false;
    }   
}

void Bubble::FreeBubbleUpdate(Bubble *bubble, float elapsedTime)
{
    bubble->m_position.x += bubble->m_velocity.x * elapsedTime;
    bubble->m_position.y += bubble->m_velocity.y * elapsedTime;
    bubble->PreventOutbound(elapsedTime);
    bubble->m_particleTrail->posX = bubble->m_position.x;
    bubble->m_particleTrail->posY = bubble->m_position.y;
}

void Bubble::BornBubbleUpdate(Bubble *bubble, float elapsedTime)
{
    bubble->m_workTime += elapsedTime;

    if (bubble->m_workTime > Bubble::BornTime)
    {
        bubble->m_workTime = 0;
        bubble->m_pfnBubbleUpdate = Bubble::LiveBubbleUpdate;
    }
    else
    {
        bubble->m_radius = (bubble->m_workTime / Bubble::BornTime) * Bubble::DiffRadius + Bubble::StartRadius;
    }
}

void Bubble::LiveBubbleUpdate(Bubble *bubble, float elapsedTime)
{
    bubble->m_workTime += elapsedTime;

    if (bubble->m_workTime > Bubble::LiveTime)
    {
        bubble->m_workTime = 0;
        bubble->m_pfnBubbleUpdate = Bubble::FadeBubbleUpdate;
    }
}

void Bubble::FadeBubbleUpdate(Bubble *bubble, float elapsedTime)
{
    bubble->m_workTime += elapsedTime;

    if (bubble->m_workTime > Bubble::FadeTime)
    {
        bubble->m_workTime = 0;
        bubble->m_isAlive = false;
        bubble->m_pfnBubbleUpdate = NULL;
    }
    else
    {
        float progress = (1.0f - bubble->m_workTime / Bubble::BornTime); 

        bubble->m_radius = progress * (Bubble::DiffRadius + Bubble::StartRadius);
        bubble->m_color.alpha = (unsigned char)(255 * progress);
    }
}

void Bubble::PreventOutbound(float elapsedTime)
{
    bool makeStep = false;

    if (m_position.x < Bubble::Bound.xStart ||
        m_position.x > Bubble::Bound.xEnd)
    {
        m_velocity.x = -m_velocity.x;
        makeStep = true;
    }

    if (m_position.y < Bubble::Bound.yStart ||
        m_position.y > Bubble::Bound.yEnd)
    {
        m_velocity.y = -m_velocity.y;
        makeStep = true;
    }

    if (makeStep)
    {
        m_position.x += m_velocity.x * elapsedTime;
        m_position.y += m_velocity.y * elapsedTime;
    }
}

void Bubble::Update(float elapsedTime)
{
    if (m_isAlive)
    {
        m_pfnBubbleUpdate(this, elapsedTime);
    }
}

void Bubble::Reset()
{
    m_velocity.x = Random(20.0f, 100.0f);
    m_velocity.y = Random(20.0f, 100.0f);
    m_velocity = m_velocity.Rotate(Random(0.0f, 2.0f * math::PI));

    m_position.x = Random(Bubble::Bound.xStart, Bubble::Bound.xEnd);
    m_position.y = Random(Bubble::Bound.yStart, Bubble::Bound.yEnd);

    m_color.red = Random(255);
    m_color.green = Random(255);
    m_color.blue = Random(255);
    m_color.alpha = 255;

    m_workTime = 0.0f;
    m_radius = Bubble::StartRadius;
    m_isAlive = true;
    m_isExploded = false;
    m_pfnBubbleUpdate = Bubble::FreeBubbleUpdate;

    m_particleTrail = Bubble::Effects.AddEffect("Trail");
    m_particleTrail->posX = m_position.x;
    m_particleTrail->posY = m_position.y;
    m_particleTrail->Reset();
}

void Bubble::Explode()
{
    m_isExploded = true;
    m_particleTrail->Finish();
    m_pfnBubbleUpdate = Bubble::BornBubbleUpdate;
    ParticleEffect *effect = Bubble::Effects.AddEffect("Explode");
    effect->posX = m_position.x;
    effect->posY = m_position.y;
    effect->Reset();
}


void BubbleGame::Initialize()
{
    Bubble::FinishAllEffects();

    m_bubbles.resize(m_pointCount);
    m_explodedIndices.clear();
    m_freeIndicies.clear();

    for (int i = 0; i < (int)m_bubbles.size(); i++)
    {
        m_bubbles[i].Reset();
        m_freeIndicies.push_back(i);
    }   
}

void BubbleGame::CheckInteractions(int explodedBubbleIndex)
{
    vector<int>::iterator item = m_freeIndicies.begin();

    while (item != m_freeIndicies.end())
    {
        if (Bubble::IsIntersect(&m_bubbles[*item], &m_bubbles[explodedBubbleIndex]))
        {
            m_bubbles[*item].Explode();
            m_explodedIndices.push_back(*item);
            item = m_freeIndicies.erase(item);
        }
        else
        {
            item++;
        }
    }
}

void BubbleGame::CheckInteractions()
{
    vector<int>::iterator item = m_explodedIndices.begin();

    while (item != m_explodedIndices.end())
    {
        CheckInteractions(*item);
        item++;
    }
}

void BubbleGame::RemoveDeadBubblesIndices()
{
    vector<int>::iterator item = m_explodedIndices.begin();

    while (item != m_explodedIndices.end())
    {
        if (!m_bubbles[*item].IsAlive())
        {
            item = m_explodedIndices.erase(item);
        }
        else
        {
            item++;
        }
    }

    if (m_explodedIndices.size() == 0)
    {
        m_isActive = false;     
        Initialize();
    }
}

BubbleGame::BubbleGame(int pointCount, float bornTime, float liveTime, const FRect *bound)
{
    Bubble::Bound = *bound;
    Bubble::BornTime = bornTime;
    Bubble::FadeTime = bornTime;
    Bubble::LiveTime = liveTime;
    Bubble::StartRadius = 4.0f;
    Bubble::DiffRadius = 20.0f;

    m_pointCount = pointCount;
    m_isActive = false;
    m_bubbles = vector<Bubble>(pointCount);
    m_bubbleTexture = Core::resourceManager.getTexture("Circle");   

    m_explodedIndices.reserve(pointCount + 1);
    m_freeIndicies.reserve(pointCount + 1);

    Initialize();
}

BubbleGame::~BubbleGame()
{
    delete m_bubbleTexture; m_bubbleTexture = NULL;
}

void BubbleGame::Trigger(const IPoint &position)
{
    if (!m_isActive)
    {
        m_isActive = true;

        Bubble explodedBubble;
        explodedBubble.Reset();
        explodedBubble.m_position.x = (float)position.x + Bubble::DiffRadius;
        explodedBubble.m_position.y = (float)position.y - Bubble::DiffRadius;
        explodedBubble.Explode();       

        m_bubbles.push_back(explodedBubble);
        m_explodedIndices.push_back((int)(m_bubbles.size() - 1));
    }
}

void BubbleGame::Update(float elapsedTime)
{
    for (int i = 0; i < (int)m_bubbles.size(); i++)
    {
        m_bubbles[i].Update(elapsedTime);
    }

    if (m_isActive)
    {
        CheckInteractions();
        RemoveDeadBubblesIndices();
    }
}

void BubbleGame::Draw()
{
    Bubble::Effects.Draw();

    m_bubbleTexture->Bind();

    for (int i = 0; i < (int)m_bubbles.size(); i++)
    {
        if (m_bubbles[i].IsAlive())
        {
            Render::device.SetCurrentColor(m_bubbles[i].m_color);

            Render::DrawRect(
                (int)(m_bubbles[i].m_position.x - m_bubbles[i].m_radius), 
                (int)(m_bubbles[i].m_position.y - m_bubbles[i].m_radius), 
                (int)(m_bubbles[i].m_radius * 2.0f),
                (int)(m_bubbles[i].m_radius * 2.0f));
        }
    }   
}

Answer 1

"Bad style" can mean a lot of things, but the worst is, it is pretty subjective what is good and bad style. IMHO, unless they gave you some literature of what they consider "good style" by their policy, it was pretty unfair to disqualify you with such a reason. Furthermore, did they really mean code style or code design? There is a slight blur between the two at some places.

Here are some things that you did but they might have not liked:

1. Even though you only have two classes, you are already starting to play around with the 'friend' attribute. 'friend' is nice hack in a large project where a redesign is a no-go, but in a project of this scale people will wonder why you didn't just come up with a design that doesn't require 'friend'. After all, if you only have two classes and one has access to the private members of the other, why bother making them private at all?
2. If you really want to go by The Book of OOP design, you should not access member variables of other classes directly, but use getter and setter methods instead. I often find this unnecessary and impractical, but some just love it.
3. You use static variables. Although they are mostly harmless with built-in type (eg. floats) as in your case, but more often than not, in complex projects and custom types you ARE going to run into hard-to-debug problems caused by static initialization order. I always design to avoid static variables, it simply saves a lot of headaches in the future.
4. You have more than one class defined in the same header. Again, you can do it, but it is not considered "clean".
5. It seems you have a Reset method on your bubbles, which you use to initialize them. You should probably call it from a constructor instead of relying on an external class to call Reset upon construction.
6. Some also prefer initializer lists in constructors, since on large arrays of simple objects (like bubbles?) they can actually give you a better performance.
7. What the others said are also true:
   1. Prefixing member varibales with m_ is not to everybody's liking. Some do it simply with an underscore, some mark member variables by casing differently, and some do not mark them at all.
   2. 'using namespace' in a header is a bad idea, it will lead to name collisions and will mess up the auto-completion feature of IDEs.
   3. Avoid names reserved by the standard/compiler.
   4. If you want to prevent copying an objetc, you need to hide/rewrite both the copy constructor AND and the assignment operator. Only one will leave you prone to errors.

Answer 2

The identifier __ BUBBLEGAME_H __ is reerved by the implementation in all contexts. The exact rules are complex see here. But shorthand never use double underscore never start an identifier with underscore (unless you know the exact rules and can remember them)

Never put a using declaration in a header file:

using namespace std;
using namespace Render;

I prefer not to even put them in a source file. But absolutely never put them in the header file. Anybody that uses your header file is now pulling stuff into another namespace and they may nor realize this without reading the code.

I don't think your usage of friend is very good. You are moving work from Bubble to Bubble Game that would be better encapsulated by the Bubble. In fact I think you can generalize Bubble with an interface so that other classes can be derived from the interface and still work with BubbleGame.

Not sure why you have or need some many public static members.

Why are you passing pointers here?

bool Bubble::IsIntersect(const Bubble *left, const Bubble *right)

Pass by const reference. You use pointers everywhere. Pointer are a big no-no as they can potentially be NULL (thus you should validate they are NULL).

Use standard algorithms to help redability:

while (item != m_explodedIndices.end())
{
    CheckInteractions(*item);
    item++;
}

Can be written as:

 std::for_each(m_explodedIndices.begin(), m_explodedIndices.end(), std::bind(CheckInteractions));

Manual resource management is a definite easy spot that this is not real C++ code (more loke C with classes). Learn to use RAII correctly.

delete m_bubbleTexture; m_bubbleTexture = NULL;

Answer 3

I'll preface my review by saying: The grading of code style is very subjective.

That said:

• You have a using namespace directive in your header file. Never do this.
• You have the using namespace std; directive in your header file. Never EVER do this.
• You have the access modifiers in the incorrect order, declare your access modifier sections in public:, protected:, then private:.
• Your public static member variables look like functions at a glance. Member variables should be declared with the first word in lowercase.
• You use a full if block to return true or false, just return the result of the expression: return (x <= y)
• Your bracing style may not conform to company standards. Always check to see if your company has a code style document that must be followed.

Answer 4

Well. First of all look at your class Bubble. It includes logic (...Update methods) and presentation (Color, you hardcoded circle drawing) and metainfo (static variables). Mixing these things is bad because when you'll be asked for change Bubble to Diamond you'll have to change whole code, when you'll need to add even minor-little-tiny change you'll have to change loads of your code. You need to divide Bubble into several classes because it violates Single Responsibility Principle. Also take a look at Model-View-Controller

No abstractions at all, only implementation. Code is hardly extensible.

friend class BubbleGame; Bad style because violates encapsulation. Nobody should even know what inside Bubble. It will be hard to find exact place where Bubble object change it's state.

const keyword not used at all. No const methods, no constant objects.

Static variables. using in header file.

Hmm, main violations described. So check out a "Clean Code: A Handbook of Agile Software Craftsmanship"

Answer 5

Two underscores reserved for compilers, so it is bad header guard: #ifndef __BUBBLEGAME_H__

Using using namespace ... directive in header is not recommended, you may use it in source file.

Very much static variables, but i cant see, that you really need all these variables as static. You may create else one class (BubbleConfig, or smth like this) for bubble configuration and pass it to Bubble.

Answer 6

For me the problem with your code is your use of

typedef void (*pfnBubbleUpdate)(Bubble *bubble, float elapsedTime);

and the associated functions:

static void XXXXBubbleUpdate(Bubble *bubble, float elapsedTime);

You should make them member functions, e.g.:

void Bubble::FreeBubbleUpdate(float elapsedTime)
{
    m_position.x += m_velocity.x * elapsedTime;
    m_position.y += m_velocity.y * elapsedTime;
    PreventOutbound(elapsedTime);
    m_particleTrail->posX = m_position.x;
    m_particleTrail->posY = m_position.y;
}

Instead of explicitly setting the pfn in your code, you should make the decision as to which function to call ONLY in your Update() function: I'd do it using a state member var, which makes debugging a lot easier:

enum BubbleState {
    BORN,
    ...
    NUM_STATES
} m_state;

Now in Update() I'd use a switch to select the appropriate update member function:

switch (m_state) {
case BORN: BornBubbleUpdate(elapsedTime);
...

or, if that led to performance issues, I'd make an array of pointers to member functions (see http://www.goingware.com/tips/member-pointers.html) and use the state's value as an index:

typedef void (Bubble::*UpdateFunc)(float elapsedTime);
UpdateFunc m_UpdateFuncArray[NUM_STATES];
...
(*(m_UpdateFuncArray[m_state]))(elapsedTime);

The last example is almost like hand-coding run-time polymorphism -- a very similar thing goes on behind the scenes when you use RTTI.

In a nutshell: There's nothing wrong with achieving polymorphism through function pointers, but you really ought to make them MEMBER function pointers.

Answer 7

A few tiny comments :

In IsIntersect :

• You don't have to use abs for xDistance and yDistance as they will be squared before being used anyway.
• All the variables can be defined as const
• At the end, you can "return (distance <= explodeDistance);"

In FadeBubbleUpdate :

• progress can be const

And more generally :

• I LOVE this game!

Answer 8

You've forbid copying by closing copying constructor in private section, but what about

const BubbleGame& operator=(const BubbleGame&) ?

It still allows to copy your BubbleGame object. Why is this bad? - You have a pointer to a texture here, which will be simply copied with the same address.

The same is for the Bubble class.

Answer 9

The whole deal with the m_pfnBubbleUpdate really has no place in a OOP design. Why do you want to replace class method in such way during run time? You should either make abstract Bubble class and derive different Bubble types from it, or use a single BubbleUpdate function doing different things depending on the Bubble's state.