patterncppMinor
Simple chain reaction game
Viewed 0 times
gamesimplereactionchain
Problem
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 m_bubbles;
vector m_explodedIndices;
vector m_freeIndicies;
Texture *m_bubbleTexture;
bool m_isActive;
int m_pointCount;
protected:
void Initialize();
void CheckInter
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 m_bubbles;
vector m_explodedIndices;
vector m_freeIndicies;
Texture *m_bubbleTexture;
bool m_isActive;
int m_pointCount;
protected:
void Initialize();
void CheckInter
Solution
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:
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?
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:
Can be written as:
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.
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;Code Snippets
using namespace std;
using namespace Render;bool Bubble::IsIntersect(const Bubble *left, const Bubble *right)while (item != m_explodedIndices.end())
{
CheckInteractions(*item);
item++;
}std::for_each(m_explodedIndices.begin(), m_explodedIndices.end(), std::bind(CheckInteractions));delete m_bubbleTexture; m_bubbleTexture = NULL;Context
StackExchange Code Review Q#4102, answer score: 7
Revisions (0)
No revisions yet.