8
\$\begingroup\$

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));
        }
    }   
}
\$\endgroup\$
1
  • \$\begingroup\$ I'm a C# dev and not familiar with C++ standard practices, but to me I'm thinking the 'style' is probably not the issue as it all appears clean and consistent. They probably had more issue with the design structure and you maybe did non-standard/unnecessary/dangerous things unknowingly (such as I would if I tried writing C++). Though maybe it's good style in C++ (I wouldn't know), I know the m_ will get you flogged outside of C++ for certain. \$\endgroup\$ Aug 15, 2011 at 12:28

9 Answers 9

6
\$\begingroup\$

"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.
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Few problems with the comments: Your concept of the friend is misguided see here. If you want to go by true OO Get/Set is bad design. It binds you public interface to an implementation. You r public interface should be actions that modify your object. If you are getting the members to do an action maybe that action should be part of the class. Not a good idea to use an underscore to prefix an identifier. Do you know what the next character can/can not be in a class context? \$\endgroup\$ Aug 15, 2011 at 14:36
  • \$\begingroup\$ I agree on your concept of _ and getters/setters, I even said I am not a fan of the latter myself. I just wrote down what many others think. However, I do not think my concept of friend is misguided. I read the thread you linked to and it properly explains that it is to extend the public interface of existing classes. But if you are in charge of the class hierarchy design, you can design it so that you make the friend stuff actual part of the public interface. Most times this is possible. Since the original posted had this oppurtunity but didn't do so, I considered this a slight design error. \$\endgroup\$
    – ultimA
    Aug 15, 2011 at 15:28
  • 1
    \$\begingroup\$ I agree with you characterization of the OP usage of friend as bad. I just think your 'friend' is nice hack in a large project is an incorrect description of how friend should be used. \$\endgroup\$ Aug 15, 2011 at 16:16
  • \$\begingroup\$ Thanks for the excellent comments. Concerning some of your quotes: m_ is used in their sample code of engine and I decided to write my code in the same way. For me "friend" seems a nice solution for game development, where I want to give access only for inner classes, like the keyword "internal" in C#. \$\endgroup\$
    – shadeglare
    Aug 16, 2011 at 4:06
  • \$\begingroup\$ Just to clarify, 'internal' in C# is something completely different. It is used make methods/classes accessbile only to consumers in the same assembly. There is no such thing in C++, except for compiler-specific extensions for marking public/private interfaces in shared objects. 'friend' in C++ will make all members of the other class visible and will violate encapsulation and data hiding. \$\endgroup\$
    – ultimA
    Aug 16, 2011 at 8:11
7
\$\begingroup\$

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;
\$\endgroup\$
4
\$\begingroup\$

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.
\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot. I never knew about the namespace directive issue in header files. \$\endgroup\$
    – shadeglare
    Aug 16, 2011 at 3:48
3
\$\begingroup\$

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"

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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!
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ It isn't a nice way use a single method in this case. I will sink in many "if"s. I thought to define a method pointer, but a simple function pointer seems better in this task. And if you use too many OOP in game development your games will be suck because of performance issues. \$\endgroup\$
    – shadeglare
    Aug 16, 2011 at 5:06
  • 1
    \$\begingroup\$ Well, you've got "bad style" answer so far, not a "bad performance" one. You don't usually use speed hacks in the job interview, code clarity and quality is much more important. \$\endgroup\$
    – Alexvn
    Aug 16, 2011 at 5:32
  • \$\begingroup\$ I think n-levels of if are even worse then to use function pointers. And I was given a game engine with not a good performance at all. It would be strange if I'd sent a low-performance solution and said them: "You have a slow engine and I can't get a nice performance with your input data". \$\endgroup\$
    – shadeglare
    Aug 16, 2011 at 5:43
  • \$\begingroup\$ What levels of if you are talking about? A simple switch statement would do just fine. My point is that performance means absolutely nothing if your code isn't even accepted for the review. As a side point, did you actually try to measure the real performance gain or you just think your solution is better? \$\endgroup\$
    – Alexvn
    Aug 16, 2011 at 6:26
  • \$\begingroup\$ The first variant I made was with a single method where were many ifs. Then I made the variant that you can see above and after getting better performance I'd selected the second. \$\endgroup\$
    – shadeglare
    Aug 16, 2011 at 6:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.