2
\$\begingroup\$

This is my example for a 2D Space Shooter coded in C++ and I would like to have some opinions about the game code and general readability (and how to improve it).

The code should be readable for other trainees since this is meant to become an exercise which is supposed to show trainees how to work with foreign libraries. The game is based on my own little 2D game engine which features gdiplus. The exercise is made for trainees with advanced C++ knowledge.

Not all parts of the code are included (pixelutility.h and the library files which are implementing the mango::features are left out), so please don't focus on the naming of the mango::features (I don't have the possibility to use a full C++11 compatible IDE so I implemented some of the C++11 or other convenient features for myself in that library, since I don't want to throw boost at that project).

main.cpp:

#include <trymacro.h>

#include "engine.h"
#include "input.h"

void resizeConsole()
{
    HWND console = GetConsoleWindow();
    RECT r;
    GetWindowRect(console, &r);
    MoveWindow(console, r.left, r.top, 640, 640, TRUE);
}

mango::int32 main()
{
    M_TRY
    {
        resizeConsole();

        engine::Game::game_handle game = new engine::Game();
        game->Play();
    }
        M_CATCH("main.txt")
}

//M_TRY {} M_CATCH(main.txt) --> try catch block to catch 
//mango::exception:: and critical errors and logging them to main.txt.

typedefs.h:

#ifndef GUARD_MANGO_TYPEDEFS_H
#define GUARD_MANGO_TYPEDEFS_H

#define NO_THREAD_ALLOC //disables thread_safety in my customallocator for better performance since this project is single threaded and locks are not needed (will be removed for multithreaded implementation)
#include <Gaming/pixelutility.h>

namespace engine
{
    typedef const mango::int32                      const_size;
    typedef mango::archive::thread_pool             thread_pool;
    typedef mango::shared_ptr<thread_pool>          pool_ptr;
    typedef mango::int32                            colour;
    typedef mango::int32                            counter;
    typedef mango::int64                            large_counter;
    typedef mango::delta_timer                      timer;
    typedef double                                  seconds;
    typedef graphic::KatCoord                       point;

    typedef graphic::Character                      player;
    typedef graphic::Character                      actor;
    typedef graphic::Character                      enemy;
    typedef graphic::Character                      effect;
    typedef graphic::Character                      background;

    typedef mango::shared_ptr<player>               player_handle;
    typedef mango::shared_ptr<actor>                actor_handle;
    typedef mango::shared_ptr<enemy>                enemy_handle;
    typedef mango::shared_ptr<effect>               effect_handle;
    typedef mango::shared_ptr<background>           background_handle;

    typedef std::string                             mesh_name;

    typedef mango::vector<actor_handle>             actor_pipeline;
    typedef mango::semimap<mesh_name, actor_handle> sprite_pipeline;
    typedef mango::semimap<effect_handle, counter>  effect_pipeline;

    typedef graphic::GraphicController              graphic_device;
    typedef mango::shared_ptr<graphic_device>       graphic_handle;

    typedef bool                                    flag;

    typedef mango::Byte                             key;
    typedef mango::queued_mutex                     lock;
    typedef mango::shared_ptr<lock>                 lock_handle;
    typedef mango::lock_guard<lock>                 lock_guard;
} 

#endif//GUARD_MANGO_TYPEDEFS_H

input.h:

#ifndef GUARD_MANGO_INPUT_H
#define GUARD_MANGO_INPUT_H

#include "typedefs.h"

namespace input
{
    static const engine::key W       = 'w';
    static const engine::key A       = 'a';
    static const engine::key S       = 's';
    static const engine::key D       = 'd';

    static const engine::key SPACE   = ' ';

    static const engine::key SHIFTX  = 'X';
    static const engine::key DEFAULT = '~';

    namespace actions
    {
        enum action
        {
            up,
            down,
            left,
            right,
            shoot,
            invalid
        };
    }

    typedef typename actions::action action;

    class InputController
    {
    public: //Typedefs
        typedef input::actions::action                     player_action;
        typedef mango::semimap<engine::key, player_action> input_map;

    public: //Functions
        InputController(engine::key up, engine::key down, engine::key left, engine::key right, engine::key shoot)
        {
            m_KeyMapping.add(up,    actions::up);
            m_KeyMapping.add(down,  actions::down);
            m_KeyMapping.add(left,  actions::left);
            m_KeyMapping.add(right, actions::right);
            m_KeyMapping.add(shoot, actions::shoot);
        }

        player_action KeyPressed(engine::key in)
        {
            return (mango::npos != m_KeyMapping.find(in)) ? m_KeyMapping.at(in) : actions::invalid;
        }

    private: //Members
        input_map m_KeyMapping;
    };

    engine::key getKeyPress()
    {
        return mango::archive::mgetch();
    }
}

namespace engine
{
    typedef input::InputController                              input_device;
    typedef mango::shared_ptr<input_device>                     input_handle;
}


#endif//GUARD_MANGO_INPUT_H

engine.h:

#ifndef GUARD_MANGO_ENGINE_H
#define GUARD_MANGO_ENGINE_H

#include "typedefs.h"
#include "input.h"

namespace engine
{
    class Game
    {
    public: //Typedefs
        typedef mango::shared_ptr<Game> game_handle;

    public: //Functions
        Game()
            : m_IsRunning(true),                                        m_UpdateRate(1.0 / 100),   
              m_PlayerAction(input::actions::invalid),                  m_ActionCounter(0), m_SpawnRate(2.4), 
              m_UpdateCount(0),                                         m_Score(0)        
        {
            m_Graphics = new engine::graphic_device(GetConsoleWindow(), engine::point(640, 640));
            m_Input    = new engine::input_device(input::W, input::S, input::A, input::D, input::SPACE);

            LoadSprite("player", 10, 295, 470, 50, 70); //PlayerShip Frames 10, Initial PlayerPosition (295|470), PlayerResolution 50px70p

            LoadBackground();
        }

        void Play()
        {
            srand((mango::uns32)time(nullptr));
            m_Timer.start();
            m_ShotTimer.start();
            m_SpawnTimer.start();
            m_DifficultyTimer.start();

            engine::key input = input::DEFAULT;

            while (input != input::SHIFTX && m_IsRunning)
            {
                if (mango::archive::mkbhit())
                {
                    input = input::getKeyPress();
                    KeyPressed(input);
                }

                Frame();
            }
        }

    private:

        bool SpawnCollision(mango::int32 x)
        {
            for (mango::int32 currentActor = 0; m_Actors.size() > currentActor; ++currentActor)
            {
                engine::point x1 = engine::point(x - 5 , 5);
                engine::point x2 = engine::point(x + 20, 5); 

                bool containsX1orX2 = m_Actors.at(currentActor)->Contains(x1) || m_Actors.at(currentActor)->Contains(x2);

                if (containsX1orX2)
                {
                    return true;
                }
            }

            return false;
        }

        void printScore()
        {
            //TODO:
        }

        void Frame()
        {
            Update();
            Collision();
            Draw();
            Present();
            ClearPipelines();
            SpawnEnemys();
            SetSpawnRate();
        }

        void SetSpawnRate()
        {
            const engine::seconds MINSPAWNRATE = 0.3;
            const engine::seconds INCREMENTATIONINTERVAL = 7.0;
            const engine::seconds SPAWNRATEINCRFACTOR = -0.05;

            if (MINSPAWNRATE <= m_SpawnRate)
            {
                m_DifficultyTimer.tick();
                if (INCREMENTATIONINTERVAL <= m_DifficultyTimer.totalTime())
                {
                    m_SpawnRate += SPAWNRATEINCRFACTOR;
                    ResetTimer(m_DifficultyTimer);
                }
            }
        }

        void SelectEnemyType(mango::int32 x)
        {
            mango::int32 enemyType = rand() % 6 + 64; //A-E
            std::string name = "enemy";
            name.push_back(enemyType);

            if ('A' == enemyType || 'C' == enemyType)
            {
                LoadEnemy(name, 1, x, -5, 40, 40); //40 - 40 --> enemyA and enemyC sprite are at the same resolution --> 40px40p 
            }
            if ('B' == enemyType || 'E' == enemyType)
            {
                LoadEnemy(name, 1, x, -5, 25, 50); //25 - 50 --> enemyB and enemyE sprite are at the same resolution --> 25px50p 
            }
            if ('D' == enemyType)
            {
                LoadEnemy(name, 1, x, -5, 30, 54); //30 - 54 --> enemyD --> 30px54p 
            }
        }

        void SpawnEnemys()
        {
            const mango::int32 MAXACTORS = 50;

            m_SpawnTimer.tick();
            if (m_SpawnRate <= m_SpawnTimer.totalTime() &&m_Actors.size() < MAXACTORS)
            {
                ResetTimer(m_SpawnTimer);

                mango::int32 x = 0;

                do
                {
                    x = rand() % 550 + 5; //5-550 Spawn Frame
                } while (SpawnCollision(x));

                SelectEnemyType(x);             
            }
        }

        static bool ProcessCollision(engine::actor_handle shot, engine::sprite_pipeline* actors, engine::actor_pipeline* shots)
        {
            const mango::int32 MAXENEMY_Y = 650;
            const mango::int32 MAXSHOT_Y = -10;

            for (mango::int32 currentActor = 0; actors->size() > currentActor; ++currentActor)
            {
                engine::point x1 = shot->GetPosition();
                engine::point x2 = shot->GetPosition();

                x2.X += 10;

                bool bothAreVisible = shot->GetVisibility() && actors->at(currentActor)->GetVisibility();
                bool containsX1orX2 = actors->at(currentActor)->Contains(x1) || actors->at(currentActor)->Contains(x2);
                bool isNoOtherShot  = 0 == shots->find(actors->at(currentActor));

                if(bothAreVisible && containsX1orX2 && isNoOtherShot) //Remove hit and ship if hit
                {
                    actors->at(currentActor)->SetVisibility(false);
                    shot->SetVisibility(false);
                }
                if (MAXENEMY_Y <= actors->at(currentActor)->GetPosition().Y) //Remove ship if out of screen
                {
                    actors->at(currentActor)->SetVisibility(false);
                }
                if (MAXSHOT_Y >= x1.Y) //Remove shot if out of screen
                {
                    shot->SetVisibility(false);
                }
            }

            return true;
        }

        void Collision()
        {
            for (mango::int32 currentShot = 0; m_Shots.size() > currentShot; ++currentShot)
            {
                ProcessCollision(m_Shots.at(currentShot), &m_Actors, &m_Shots);
            }
        }

        void ClearPipelines()
        {
            for (mango::int32 currentShot = 0; m_Shots.size() > currentShot; ++currentShot)
            {
                if (!m_Shots.at(currentShot)->GetVisibility())
                {
                    m_Actors.remove(m_Shots.at(currentShot));
                    m_Shots.erase(currentShot);

                    --currentShot;
                }
            }

            for (mango::int32 currentActor = 0; m_Actors.size() > currentActor; ++currentActor)
            {
                const mango::int32 MAXENEMY_Y = 650;

                if (MAXENEMY_Y <= m_Actors.at(currentActor)->GetPosition().Y)
                {
                    engine::actor_handle gameOver = new engine::actor(engine::point(0, 150), 600, 200);
                    gameOver->LoadAnimation(0, "game_over", 1);

                    m_Graphics->ClearBuffer();
                    m_Graphics->Draw((engine::actor*)m_Background);
                    m_Graphics->Draw((engine::actor*)gameOver);
                    m_Graphics->Present();

                    mango::csleep(3000); //3 Seconds

                    m_IsRunning = false;
                    return;
                }

                if (!m_Actors.at(currentActor)->GetVisibility())
                {
                    SpawnEffect(m_Actors.at(currentActor), "explosion");
                    m_Actors.remove(m_Actors.key_at(currentActor));

                    --currentActor;
                    m_Score += 100;
                }
            }
        }

        void Shoot(engine::key key)
        {
            const mango::int32 ACTIONCOUNTERINCRFACTOR = 30;

            if (input::actions::shoot == m_Input->KeyPressed(key))
            {
                SpawnShot("player");

                if ((0 != m_ActionCounter && input::actions::right == m_PlayerAction))
                {
                    m_PlayerAction = input::actions::right;
                    m_ActionCounter = ACTIONCOUNTERINCRFACTOR;
                }
                if ((0 != m_ActionCounter && input::actions::left == m_PlayerAction))
                {
                    m_PlayerAction = input::actions::left;
                    m_ActionCounter = ACTIONCOUNTERINCRFACTOR;
                }
                if ((0 != m_ActionCounter && input::actions::up == m_PlayerAction))
                {
                    m_PlayerAction = input::actions::up;
                    m_ActionCounter = ACTIONCOUNTERINCRFACTOR;
                }
                if ((0 != m_ActionCounter && input::actions::down == m_PlayerAction))
                {
                    m_PlayerAction = input::actions::down;
                    m_ActionCounter = ACTIONCOUNTERINCRFACTOR;
                }
            }
        }

        void Move(engine::key key)
        {
            const mango::int32 ACTIONCOUNTERINCRFACTOR = 30;

            if (input::actions::left == m_Input->KeyPressed(key))
            {
                if (0 == m_ActionCounter || input::actions::left != m_PlayerAction)
                {
                    m_PlayerAction = input::actions::left;
                    m_ActionCounter = ACTIONCOUNTERINCRFACTOR;
                }
            }
            if (input::actions::right == m_Input->KeyPressed(key))
            {
                if (0 == m_ActionCounter || input::actions::right != m_PlayerAction)
                {
                    m_PlayerAction = input::actions::right;
                    m_ActionCounter = ACTIONCOUNTERINCRFACTOR;
                }
            }
            if (input::actions::up == m_Input->KeyPressed(key))
            {
                if (0 == m_ActionCounter || input::actions::up != m_PlayerAction)
                {
                    m_PlayerAction = input::actions::up;
                    m_ActionCounter = ACTIONCOUNTERINCRFACTOR;
                }
            }
            if (input::actions::down == m_Input->KeyPressed(key))
            {
                if (0 == m_ActionCounter || input::actions::down != m_PlayerAction)
                {
                    m_PlayerAction = input::actions::down;
                    m_ActionCounter = ACTIONCOUNTERINCRFACTOR;
                }
            }
        }

        void KeyPressed(engine::key key)
        {
            Shoot(key); 
            Move(key);
        }


        void Draw()
        {
            m_Graphics->ClearBuffer();

            m_Graphics->Draw((engine::actor*)m_Background);
            m_Graphics->Draw(m_Actors);

            for (mango::int32 currentEffect = 0; m_Effects.size() > currentEffect; ++currentEffect)
            {
                m_Graphics->Draw((engine::actor*)m_Effects.key_at(currentEffect));
            }
        }

        void UpdateEnemies()
        {
            const mango::int32 ENEMYMOTIONRATE_X = 0;
            const mango::int32 ENEMYMOTIONRATE_Y = 1;

            for (mango::int32 currentActor = 0; m_Actors.size() > currentActor; ++currentActor)
            {
                bool isNoPlayerShot = 0 == m_Shots.find(m_Actors.at(currentActor));
                bool isNotThePlayer = m_Actors.at(currentActor) != m_Actors.at(0);

                if (isNoPlayerShot && isNotThePlayer)
                {
                    m_Actors.at(currentActor)->Move(ENEMYMOTIONRATE_X, ENEMYMOTIONRATE_Y);
                }
            }
        }

        void UpdateActors()
        {
            m_Timer.tick();
            if ((m_UpdateRate) <= m_Timer.totalTime())
            {
                if (0 != m_Actors.size())
                {
                    lambda((engine::actor_handle actor) { actor->Update(); return 0; }, 1); //exp1
                    m_Actors.for_each(exp1);
                }

                ++m_UpdateCount;

                UpdatePlayer();
                UpdateEffects();
                UpdateEnemies();

                ResetTimer(m_Timer);
            }
        }

        void UpdateBackground()
        {
            const mango::int32 BGMOTIONRATE_X = 0;
            const mango::int32 BGMOTIONRATE_Y = 1;

            const mango::int32 BGUPDATETHRESHHOLD = 3;

            if (BGUPDATETHRESHHOLD == m_UpdateCount)
            {
                if (nullptr != m_Background)
                {
                    m_Background->Move(BGMOTIONRATE_X, BGMOTIONRATE_Y);

                    if (0 == m_Background->GetPosition().Y)
                    {
                        m_Background->SetPosition(engine::point(0, -600)); //Reset BG to initial position
                    }
                }

                m_UpdateCount = 0;
            }
        }

        void UpdateShots()
        {
            const mango::int32 SHOTMOTIONRATE_X = 0;
            const mango::int32 SHOTMOTIONRATE_Y = -4;

            if (m_Shots.size() > 0)
            {
                lambda((engine::actor_handle shot) { shot->Move(SHOTMOTIONRATE_X, SHOTMOTIONRATE_Y); return 0; }, 1);

                m_Shots.for_each(exp1);
            }
        }

        void UpdateEffects()
        {
            for (mango::int32 currentEffect = 0; m_Effects.size() > currentEffect; ++currentEffect)
            {       
                m_Effects.key_at(currentEffect)->Update();

                if (mango::npos == --m_Effects.at(currentEffect))
                {
                    m_Effects.remove(m_Effects.key_at(currentEffect));
                    --currentEffect;
                }
            }
        }

        void UpdatePlayer()
        {
            const mango::int32 PLAYERMOTIONRATE = 3;
            const mango::int32 NOMOTION = 0;

            const mango::int32 MINPLAYER_X = 5;
            const mango::int32 MAXPLAYER_X = 550;

            const mango::int32 MINPLAYER_Y = 5;
            const mango::int32 MAXPLAYER_Y = 470;

            if (0 != m_ActionCounter)
            {
                engine::point player = m_Actors.at(0)->GetPosition();

                if (input::actions::left == m_PlayerAction)
                {
                    if (player.X >= MINPLAYER_X)
                    {
                        MovePlayer( -PLAYERMOTIONRATE, NOMOTION);
                    }
                    --m_ActionCounter;
                }
                if (input::actions::right == m_PlayerAction)
                {
                    if (player.X <= MAXPLAYER_X)
                    {
                        MovePlayer(PLAYERMOTIONRATE, NOMOTION);
                    }
                    --m_ActionCounter;
                }
                if (input::actions::up == m_PlayerAction)
                {
                    if (player.Y >= MINPLAYER_Y)
                    {
                        MovePlayer(NOMOTION, -PLAYERMOTIONRATE);
                    }
                    --m_ActionCounter;
                }
                if (input::actions::down == m_PlayerAction)
                {
                    if (player.Y <= MAXPLAYER_Y)
                    {
                        MovePlayer(NOMOTION, PLAYERMOTIONRATE);
                    }
                    --m_ActionCounter;
                }
            }
            else
            {
                m_PlayerAction = input::actions::invalid;
            }
        }

        void Update()
        {
            UpdateActors();
            UpdateBackground();
            UpdateShots();
        }

        void Present()
        {
            m_Graphics->Present();
            printScore();
        }

        void ResetTimer(engine::timer& timer)
        {
            timer.stop();
            timer.reset();
            timer.start();
        }

        void LoadSprite(const engine::mesh_name& name, mango::int32 frames, mango::int32 x, mango::int32 y, mango::int32 width, mango::int32 height)
        {
            engine::actor_handle actor;

            actor = new engine::actor(engine::point(x, y), width, height);
            actor->LoadAnimation(0, name, frames);

            m_Actors.add(name, actor);
        }

        void LoadEnemy(const engine::mesh_name& name, mango::int32 frames, mango::int32 x, mango::int32 y, mango::int32 width, mango::int32 height)
        {
            static engine::large_counter ID = 0;

            engine::actor_handle actor;

            actor = new engine::actor(engine::point(x, y), width, height);
            actor->LoadAnimation(0, name, frames);

            m_Actors.add(name + mango::to_string(ID), actor);

            ++ID;
        }

        void LoadShot(const engine::mesh_name& name, mango::int32 frames, mango::int32 x, mango::int32 y, mango::int32 width, mango::int32 height)
        {
            static engine::large_counter ID = 0;

            engine::actor_handle actor;

            actor = new engine::actor(engine::point(x, y), width, height);
            actor->LoadAnimation(0, name, frames);

            m_Shots.push_back(actor);
            m_Actors.add(name + mango::to_string(ID), actor);

            ++ID;
        }

        void LoadEffect(const engine::mesh_name& name, mango::int32 frames, mango::int32 x, mango::int32 y, mango::int32 width, mango::int32 height)
        {
            const mango::int32 EFFECTFRAMES = 5;

            engine::actor_handle actor;

            actor = new engine::actor(engine::point(x, y), width, height);
            actor->LoadAnimation(0, name, frames);

            m_Effects.add(actor, EFFECTFRAMES);
        }

        void LoadBackground()
        {
            m_Background = new engine::actor(engine::point(0, -600), 640, 1200);
            m_Background->LoadAnimation(0, "background", 1);
        }

        void MoveActor(std::string name, mango::int32 x, mango::int32 y)
        {
            m_Actors.at(name)->Move(x, y);
        }

        void MovePlayer(mango::int32 x, mango::int32 y)
        {
            m_Actors.at(0)->Move(x, y);
        }

        void SpawnShot(std::string actor_name)
        {
            m_ShotTimer.tick();

            if (0.20 <= m_ShotTimer.totalTime())
            {
                ResetTimer(m_ShotTimer);

                engine::point spawnPosition = m_Actors.at(actor_name)->GetPosition();

                if ("player" == actor_name)
                {
                    spawnPosition.X += 22;
                    spawnPosition.Y -= 25;

                    LoadShot("playerShot", 1, spawnPosition.X, spawnPosition.Y, 10, 20);
                }
            }
        }

        void SpawnEffect(actor_handle actor, std::string effect_name)
        {
            engine::point spawnPosition = actor->GetPosition();

            LoadEffect(effect_name, 5, spawnPosition.X, spawnPosition.Y, 50, 50);
        }

        void SpawnEffect(std::string actor_name, std::string effect_name)
        {
            engine::point spawnPosition = m_Actors.at(actor_name)->GetPosition();

            LoadEffect(effect_name, 5, spawnPosition.X, spawnPosition.Y, 50, 50);
        }

        input::action             m_PlayerAction;
        engine::counter           m_ActionCounter; //While Counter != 0 PlayerAction (except shooting) will be repeated for control smoothing

        engine::seconds           m_UpdateRate;
        engine::seconds           m_SpawnRate;
        engine::counter           m_UpdateCount;

        engine::timer             m_Timer;
        engine::timer             m_ShotTimer;
        engine::timer             m_SpawnTimer;
        engine::timer             m_DifficultyTimer;

        engine::sprite_pipeline   m_Actors;
        engine::graphic_handle    m_Graphics;
        engine::input_handle      m_Input;

        engine::actor_pipeline    m_Shots;
        engine::effect_pipeline   m_Effects;

        engine::background_handle m_Background;

        engine::flag              m_IsRunning;
        engine::counter           m_Score;
    };

    typedef mango::pair<engine::Game::game_handle, engine::key> input_package;
}


#endif//GUARD_MANGO_ENGINE_H

My future plans are:

  1. Split the code in header/cpp
  2. Implement fighting enemies (atm. enemies are not shooting at the player)
  3. Implement multithreading

Feel free to reuse the code where ever you like.

Here is a screenshot of the current game status:

enter image description here

the sprites are third party from the internet (opengameart, etc.)

\$\endgroup\$
  • 3
    \$\begingroup\$ I remeber writing a comment to this, but I don't see it any more. My comment was: I would not do the variable (Etc) aligning. It is a lot of work and hard to maintain: If you add something that is longer then the rest, you have to edit all lines. It is also not nice for Version management software, which will tag each line as being changed, because you only added a space. It does not improve readability imho, as you now have to scroll left to see the variable names. \$\endgroup\$ – JHBonarius Mar 30 '17 at 8:29
  • \$\begingroup\$ eh yep your comment is gone, sorry I don't know why but thanks anyway :) \$\endgroup\$ – Mango Mar 30 '17 at 8:31
  • \$\begingroup\$ I think I know why now... This is a new question! I see different code then last time, and the links to your previous projects are gone... but it hasn't been edited.... ;) \$\endgroup\$ – JHBonarius Mar 30 '17 at 8:34
5
\$\begingroup\$

Is this because of the engine you are using?

mango::int32 main()

The only standard versions of main are:

int main()
int main(int argc, char* argv[])

Why are you dynamically creating an object? Then leaking it?

        engine::Game::game_handle game = new engine::Game();
        game->Play();

Much easier to write:

        engine::Game::Game game;
        game.Play();

Naming conventions. It is more usually to have function names with an initial lower case letter. This helps us distinguish types from objects (user types normally have an initial uppercase letter).

You are using the term handle incorrectly. The term handle is used as a double indirection. It is a technique that allows somebody to delete a resource and set the the first indirection to null showing the resource been released (thus anybody else with a handle will see it has been released).

The term handle has fallen out of use as the ownership semantics of C++ provides a much better mechanism. Really what you have a various pointer types.

These seem to be badly named:

static const engine::key W       = 'w';
static const engine::key A       = 'a';
static const engine::key S       = 's';
static const engine::key D       = 'd';

static const engine::key SPACE   = ' ';

static const engine::key SHIFTX  = 'X';
static const engine::key DEFAULT = '~';

I presume these keys perform actions? I would rather name them with their action. So if you change the key bindings you don't get inconsistent code. If Itook your code but I prefer 'o' and 'p' for left right your code still has A and S scattered through the source.

static const engine::key Up          = 'w';
static const engine::key Down        = 'a';
static const engine::key Left        = 's';
static const engine::key Right       = 'd';

static const engine::key Fire        = ' ';

static const engine::key SmartBomb   = 'X';
static const engine::key Shield      = '~';

Now I can change the key bindings without making the code look weird. If I change the left right to 'o' 'p' the code still contains comparisons to Left and Right.

Also note the all uppercase identifiers are by convention reserved from macros. Please don't abuse that it can lead to some complicated issues.

Please use the more modern randum number library.

       // the ::srand and ::rand functions should be
       // considered deprecated at this point. They
       // were not that good before now we have better stuff.
       srand((mango::uns32)time(nullptr));

Overall you use dynamically allocated too much. Use automatic mariables where you can.

\$\endgroup\$
  • \$\begingroup\$ ok, well first: i always use int32, that's just personal style and should make no difference at this point, W, A, S, D are just const values that i don't have to use magic numbers/characters the are only used for the initialization. The upper case functions are a valid point, thanks for that usualy i write member functions in upper, regular functions in lower case letters, but i will change that. To the dynamic allocation point, well i use a custom allocator, you can't know that, but my heap allocations are really fast since i preallocate a lot of memory. I favor my objects on the heap. \$\endgroup\$ – Mango Mar 29 '17 at 19:27
  • \$\begingroup\$ but overall good points, thank you, i will think of it next time i code sth \$\endgroup\$ – Mango Mar 29 '17 at 19:29
  • \$\begingroup\$ By using automatic members (rather than dynamic) you increase locality. This can increase speed because you are getting better cache usage. Remember cache are built out out of lines if you use dynamic allocation each object is probably on a different line unless the object fills the line under utilizing it. If all the objects are part of the same structure you get get better cache usage and thus more items in the cache which will potentially increase speed (an import point in games). \$\endgroup\$ – Martin York Mar 29 '17 at 19:39
  • \$\begingroup\$ actually i want this to avoid false sharing since this is supposed to become a multithreaded program (correct me if i got sth. wrong) \$\endgroup\$ – Mango Mar 29 '17 at 19:42
2
\$\begingroup\$

Just as last time:

typedef typename actions::action action;

Is equal to:

using actions::action;

Like said by Loki Astari: your code is missing destructors. Using new you create objects, but you never delete them. This is considered bad coding style. Nowadays it is advised to instead use std::unique_ptr or std::shared_ptr.


My general opinion is that all the typedefs actually make the code difficult to read. I think it will become difficult to maintain. Once you switched projects some times and go back to this one, it will take you some time to figure out what types you were using again. I would advice you to use standard data types and full names as much as possible. Example: in engine.h I see that m_Shots is of the type engine::actor_pipeline, but I have to go to typedefs.h to see that that means it is a mango::vector<actor_handle>, where actor_handle seems to be of the type mango::shared_ptr<actor>, where actor is of the type graphic::Character... very confusing... I would just write:

using mango::vector;
using mango::shared_ptr;
using graphics::Character;

vector<shared_ptr<Character>> m_Shots;

Comment you code... To you the naming might seem clear now, but without explanation comments it is not always clear what is happening and why.


Constants like:

const engine::seconds MINSPAWNRATE = 0.3;
const engine::seconds INCREMENTATIONINTERVAL = 7.0;
const engine::seconds SPAWNRATEINCRFACTOR = -0.05;

These three are put on the stack every time SetSpawnRate is called. Maybe they should be static const on the top level class.


Also: put the engine.h functions implementations in engine.c.


Const correctness: if a class function/method does not modify any member variables, make it const.


Loops: e.g. in ClearPipelines():

m_Shots.size() > currentShot; ++currentShot)

These kind of micro-optimizations are no longer required with current compilers. Modern compilers will know what you want and do this for you. In the same function you write

m_Shots.at(currentShot)[...]

the at() operator uses more overhead then [], as it does range checking. So when you loop from 0 to m_Shots.size(), there is no reason to use it. This should never go out of range (or you did something wrong with multi-threading).

Actually, ClearPipelines is quite awkward. While looping over the m_Shots, you change the contents and possibly the size of the vector. You then trick the loop by decreasing the index (p.s. currentShot is the index, not an actual shot. the naming is wrong imho). You remove random elements from the vector. A vector is not really suited for this, as all elements after the removed element need to be moved. This is quite inefficient. Likely a std::list is better suited here.

With the list, you can also remove elements while iterating

for (auto iter_shots = m_Shots.begin(); iter_shots != m_Shots.end(); iter_shots++)
{
    if (!(*iter_shots)->GetVisibility())
    {
        m_Actors.remove(*iter_shots);
        m_Shots.erase(iter_shots);
    }
}

For vector loops from 0 to size() you can nowadays (C++11) used ranged for

for(auto element : vector_name) { ... }

Still magic numbers in the code, e.g. x2.X += 10;... totally unclear what is happening here.


lambda((engine::actor_handle actor) { actor->Update(); return 0; } in UpdateActors() seems weird. It seems a void functions, why should it return 0?

\$\endgroup\$
  • \$\begingroup\$ well sadly my lambda needs a retuen value... litte trade for missing it completely, I can't use c++11, static const is a good idea and actually all of my dynamic allocated objects are held by shared_ptr sometimes i just pass some of them as normal pointer, but thanks some good points \$\endgroup\$ – Mango Mar 30 '17 at 9:44
  • 1
    \$\begingroup\$ ok.... C++11 is six years old... C++14 has come since then and now we are moving to C++17... \$\endgroup\$ – JHBonarius Mar 30 '17 at 10:08
  • \$\begingroup\$ well I could use vs community but at my workplace vs2010 is still in use and vs 2010 is missing a lot sadly (shared_ptr are working at least :)) but since i code some of this as excersices for work i am stuck to older standards which is pretty annoying... but well your advice is still helpful for the future \$\endgroup\$ – Mango Mar 30 '17 at 10:11
  • \$\begingroup\$ and probably one more thing to say, my mango::vector is more a vector like object (i really should think about changing the name) erase and insert are pretty fast on that container but overall you are right, a vector would be a suboptimal choice for that kind of work \$\endgroup\$ – Mango Mar 30 '17 at 11:08
  • \$\begingroup\$ Why are you reinventing STL? why make your own vector if you have std::vector? Compilers are especially good in optimizing STL. \$\endgroup\$ – JHBonarius Mar 30 '17 at 11:45

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.