6
\$\begingroup\$

I'm particularly focused on learning object oriented design right now there's some SDL code but it does nothing save generate a window. For now I am working with text until I get a better understanding of SDL and graphics in general.

My questions are:

  1. Is this a good example of object oriented programing?
  2. What am I doing wrong / right?

Game.h

#pragma once
#define Charact 3
#include <iostream>
#include <vector>
#include "Character.h"
#include "Events.h"
#include "Window.h"
#include "Weapons.h"
#include "Inventory.h"
#include <SDL.h>
#include <Windows.h>
#include <SDL_image.h>
#include <new>
#include <map>
class Game : public Character
{
public:

    struct Room
    {
        int RoomNumber;
        int Exits;
        string Name;
        int X;
        int Y;
    };

    Game();
    int Chapter = 0;
    bool isrunning;
    std::string *description;
    enum class States{NEW_GAME, EVENT, PLAYING, QUIT};
    std::string directions;
    std::string commandR;
    string element;
    int controllers;
    States state;
    Character *Party[Charact];
    Window W;
    Room R;
    Inventory I;
    SDL_Renderer *render = nullptr;
    SDL_Texture *Sky = nullptr;
    SDL_Rect sky_rect;
    SDL_Surface *windowSurface = nullptr;
    SDL_Surface *imageSurface = nullptr;
    SDL_Event E;
    ~Game();
};

Game.cpp

#include "Game.h"

Game::Game()
{
    state = States::NEW_GAME;
    isrunning = true;

    //The Rooms Do these belong here or should there be a separate class?
    Room *R = new Room[5];

    //Intro Room.
    R[0].RoomNumber = 1;
    R[0].Name = "Small Shaft";
    R[0].Exits = 1;
    R[0].X = 100;
    R[0].Y = 500;

    //Continued Tunnel.
    R[1].RoomNumber = 2;
    R[1].Name = "Continued Shaft";
    R[1].Exits = 2;
    R[1].X = 100;
    R[1].Y = 500;



    //The Party Do these belong here or should there be a separate class?
    Party[0] = new Character{Party_members::MUSUNGO, Musungo_State::NORMAL, Status::NORMAL, Classes::SOUL_EATER, true, false, 100, 0.00, "Musungo", 1, 100, 50, 100, nullptr};  
    Party[1] = new Character{Party_members::SARUTO, Status::NORMAL, Classes::SHINIGAMI, false, false, 800, 0.00, "Saruto", 5, 30, 150, 250, nullptr};
    Party[2] = new Character{Party_members::DOKU, Status::NORMAL, Classes::DRAGON, false, false, 1000, 0.00, "Doku", 5, 25, 50, 300, nullptr};

    //Weapons Do these belong here or should there be a separate class?
    Item *Empty = new Item{"Empty", "Empty", 0, 0, 0, 0, 0, 0};
    Item *Sword = new Weapons("Sword", "A bland Sword", 0, 0, 5, 10, 3, 2, 6);
    Item *Spear = new Weapons("Spear", "A Dull Spear", 0, 0, 10, 15, 5, 10, 15);
    Item *Longsword = new Weapons("Longsword", "A basic Longsword", 0, 2, 15, 3, 5, 0, 3);
    I.Player_inventory.push_back(Empty);

    while (state != States::QUIT)
    {
        if (state == States::NEW_GAME)
        {
            Chapter = 0;
            Events *intro = new Events("Intro", "You have woken up in a cave there appears to be no exit from where you are\n You hear a voice """"Our purpose is to guide you."""" Unsure of what that was about you decide\n To press forward hoping to find an exit.", 0);       
            cout << R[Chapter].Name.c_str() << endl;;
            cin >> commandR;
        }
    }

    if (isrunning == false)
    {
       //I had a lot of this earlier that I removed to focus on the 
         Game engine. 
        SDL_DestroyWindow(W.window);
        SDL_FreeSurface(imageSurface);      
        SDL_DestroyRenderer(render);
        render = nullptr;
        W.window = nullptr;
        imageSurface = nullptr;
        windowSurface = nullptr;
    }

}




Game::~Game()
{
}

Character.h

#pragma once
#include <iostream>
#include <SDL.h>
#include "Item.h"

class Character
{

public:
    Character();
    enum class Party_members { MUSUNGO, DOKU, SARAH, SARUTO, OGUMO, KEN };
    enum class Classes { HUMAN, DHUMAN, DRAGON, DREAM_WEAVER, SHINIGAMI, SOUL_EATER};
    enum class Status { NORMAL, DEAD, POISEND, BURNED };
    enum class Fighting_styles { SWORDSMAN, MAGE, THEIF, ASSAIN, DRAGOON, GUNMAN };
    enum class Musungo_State { NORMAL, UNDEAD, GHOST };

private:
    SDL_Texture *Player;
    int _Health; 
    double _Experience;
    std::string _Name;
    int _Level; 
    double _Speed; 
    double _Magic;
    double _Strength;
    bool _isDragon;
    bool _InParty;

public: 
    Character(Party_members P, Status ST, Classes CL , bool Inparty, bool isDragon, int H, double E, std::string N, int L, double S, double M, double STR, SDL_Texture *T);
    Character(Party_members P, Musungo_State MS, Status ST, Classes CL, bool Inparty, bool isDragon, int H, double E, std::string N, int L, double S, double M, double STR, SDL_Texture *T);

    //Getters
    int GetHealth();

    double GetExp();
    std::string GetName();
    int GetLevel();
    Musungo_State MS;
    Party_members PM;
    Status ST;
    double GetSpeed();
    double GetMagic();
    double GetStrength();
    Musungo_State GetMusungoState() const;
    Party_members GetPartyMembers() const;
    Status GetStatus() const;

    //Setters
    void SetHealth(int H);
    void SetLevel(int L);
    void SetExp(double E);
    void SetSpeed(double S);
    void SetMagic(double M);
    void SetStrength(double ST);
    void SetName(std::string N);

    ~Character();
};

Character.cpp

#include "Character.h"

Character::Character()
{

}

Character::Character(Party_members P, Status ST, Classes CL, bool Inparty, bool isDragon, int H, double E, std::string N, int L, double S, double M, double STR,  SDL_Texture *T)
{
    _Health = H;
    _Experience = E;
    _Level = L;
    _Speed = S;
    _Magic = M;
    _isDragon = isDragon;
    _Name = N;      
    _Strength = STR;
    Player = T;

    if (Inparty == true)
    {
        cout << N.c_str() << endl;
    }

    if (ST == Status::NORMAL)
    {
        std::cout << N.c_str()<< ": I am Normal" << std::endl;
    }

    if (ST == Status::BURNED)
    {
        std::cout << N.c_str() <<": I am Burned Severly" << std::endl;
    }

    if (ST == Status::DEAD)
    {
        std::cout << N.c_str() <<": I am Dead" << std::endl;
    }

    if (ST == Status::POISEND)
    {
        std::cout << N.c_str() << ": I don't feel well." << std::endl;
    }

}

void Character::SetHealth(int H)
{
     _Health = H;
}

void Character::SetLevel(int L)
{
     _Level = L;    
}

void Character::SetExp(double E)
{
     _Experience = E;   
}

void Character::SetSpeed(double S)
{
     _Speed = S;    
}

void Character::SetMagic(double M)
{
     _Magic = M;    
}

void Character::SetStrength(double ST)
{
    _Strength = ST;
} 

void Character::SetName(std::string N)
{
     _Name = N; 
}

Character::Character(Party_members P, Musungo_State MS, Status ST, Classes CL, bool Inparty, bool isDragon, int H, double E, std::string N, int L, double S, double M, double STR, SDL_Texture * T)
{
    _Health = H;
    _Experience = E;
    _Level = L;
    _isDragon = isDragon;
    _Speed = S;
    _Magic = M;
    _Name  = N;
    _Strength = STR;
    Player = T;
    if (MS == Musungo_State::NORMAL)
    {
        std::cout << "Musungo: I am Alive" << std::endl;
    }

    if (MS == Musungo_State::UNDEAD)
    {
        std::cout << "Musungo: I am a corpse hunting for souls" << std::endl;
    }

    if (MS == Musungo_State::GHOST)
    {
        std::cout << "Musungo: I am a Ghost." << std::endl;
    }
}

int Character::GetHealth()
{
    return _Health;
}

double Character::GetExp()
{
    return _Experience;
}

std::string Character::GetName()
{
    return _Name;
}

int Character::GetLevel()
{
    return _Level;
}

double Character::GetSpeed()
{
    return _Speed;
}

double Character::GetMagic()
{
    return _Magic;
}

double Character::GetStrength()
{
    return _Strength;
}

 Character::Musungo_State Character::GetMusungoState() const 
{
    return MS;
}

Character::Party_members Character::GetPartyMembers() const 
{
    return PM;
}

Character::Status Character::GetStatus() const
{
    return ST;
}

Character::~Character()
{
}

Item.h

#pragma once
#include <iostream>
#include <vector>
using namespace std;
#define WEAPONS = 0x000;
#define ARMOUR = 0x001;
#define POTION = 0x002;

class Item
{

private:
    string _Name;
    string _description;

public:
    Item();
    Item(string Name, string description, int type, int ID, int Health = 0, int Speed = 0, int Strength = 0, int Magic = 0);
    enum Slots{ HEAD = 0x003, ARMS = 0x004, HAND = 0x005};
    string Name;
    string description;
    int type;
    int ID;
    int Health = 0;
    int Speed = 0;
    int Strength = 0;
    int Magic = 0;
    ~Item();
};

Item.cpp

#include "Item.h"



Item::Item()
{
    _Name = Name;
    _description = description;
}

Item::Item(string Name, string description, int type, int ID, int Health, int Speed, int Strength, int Magic)
{
}


Item::~Item()
{
}

Inventory.h

#pragma once
#include <iostream>
#include <vector>
#include "Item.h"

class Inventory
{
public:
    Inventory();
    int slot;

    std::vector <Item*> Player_inventory;
    std::vector <Item*> Equipment{};
    ~Inventory();
};
\$\endgroup\$
2
\$\begingroup\$

I have only skimmed your code, but this is what I've found so far.

Avoid macros when possible.
A constant variable inside of Game would be better than #define Charact 3. You could make the variable static if you really needed to.

The Game class should not inherit from the Character class.
This is counter-intuitive and bad design. If your Game class needs access to the Character class, then use composition instead of inheritance.

Avoid pointers whenever possible.
Why is std::string *description a pointer?
Why is Party an array of pointers?
Why is Room *R a pointer?

The Room *R can make your code confusing to read because the same class has a member variable Room R.

Now games do use pointers more often than most other software, but they use pointers for polymorphism and for passing around references that could be nullptr. None of your classes have virtual destructors, so none of them are suitable for polymorphism.

Try to use a consistent naming convention for your classes, variables, and functions.
Your Game class mixes lowercase and uppercase for member variable names.
Your Character class has leading underscores for member variable names. You should avoid leading underscores in general.

Avoid running the actual game inside your constructor. People using your class would probably want to create a Game object without getting stuck in a loop. You also have a lot of tear-down code after the while-loop in your constructor. You should learn about RAII. It's a very powerful concept in C++ and is the correct way to manage resources.

Avoid printing information in your Character constructor. You want to decouple your user interface from your business logic/data layer. This is especially important since I'm assuming you'll eventually want to use SDL for creating a graphical game rather than a command-line terminal game.

Avoid using namespace std;, especially in a header file.

There is a lot that I did not look at, so I don't know if your design is any good.

\$\endgroup\$
  • \$\begingroup\$ When you say don't run the game in the constructor do you mean the main while loop??? Should That be in main? \$\endgroup\$ – user2350585 Aug 1 '17 at 20:37
  • \$\begingroup\$ @user2350585 You could have a member function called Start() or something like that. You could call that in the main() function. \$\endgroup\$ – ncalmbeblpaicr0011 Aug 1 '17 at 21:01
  • \$\begingroup\$ @user2350585 The idea is that a constructor creates a valid, yet inactive object. Some go as far as saying that a constructor should have no code at all (save for simple assignment). \$\endgroup\$ – Aziuth Aug 2 '17 at 8:51
2
\$\begingroup\$

I don't work a lot with C++ these days, so I'm not really going into details there (except for a few important points), but this is what I've got:

  • You've got a memory leak: you're allocating space for Rooms with new[], but you're not releasing it with delete[]. Anything created with new (or new[]) must be released with delete (or delete[]). You should read up on RAII and smart pointers.
  • Use descriptive names to make your code easier to read. That, in turn, will make it easier to understand and to work with. All those single-character names may be quick and easy to type, but code is read more often than it is written - and a good IDE should make the typing part relatively easy, anyway.
  • Game should not inherit from Character. Inheritance implies an 'is-a' relationship, and I'm fairly sure that a game is not a character. Rather, a game 'has-a' character (or multiple characters) - and indeed, your game class has a Party field, so that's good.
  • Try to group related fields and functions together in your class definitions. This is especially a problem with the Game class. Character looks better, although personally I don't think grouping getters and setters together is very useful - I'd put matchin getters and setters together. Also, inner structs and enums aren't part of the state of a class, so I would keep them separate.
  • Game and Inventory expose a lot of fields publicly. This means that they can be modified by any other code, which makes it easy to introduce bugs. Try to keep the public interface of a class as small as possible, and hide implementation details. This makes it easier to ensure that a class is always in a valid state, and allows you to optimize an implementation without having to change the code that uses that class.
  • Constructors are meant to initialize an object, and destructors are for cleaning up any resources you no longer need. They're not meant to do actual work, such as running a main game loop or outputting things to a console. Move those things to separate functions, such as Game.start or Game.runMainLoop.
  • Your Character class uses setters, which is good. They don't actually perform any validity checks however. Should setting a negative or an extremely high health value be possible? If not, SetHealth should prevent it. And perhaps setting health to 0 should update other fields, such as a character's status?
  • The Character constructor writes directly to the standard output. Not only is the constructor a strange place to do so, it also ties your Character class closely to a specific kind of output. It's often a good idea to have a separate layer of code that's responsible for displaying the current game (or program) state. Classes and functions should ideally have a single responsibility, so when you need to modify something, you only need to modify code in a single place.
  • variable == false is normally written as !variable, and variable == true is simply written as variable, because variable is already either true or false.
  • You've got some typos in your enums in Character. They could make things more difficult to find in the future.

It looks like you've still got a bit of work to do to get a fully working game, so good luck with it!

\$\endgroup\$
  • \$\begingroup\$ Thanks, I'll keep alot of this in mind going forward :-) \$\endgroup\$ – user2350585 Aug 1 '17 at 21:11
2
\$\begingroup\$

Some things regarding your structure:

I'd recommend that everything that is not an algorithm but actual data is stored in files that can be read. One example would be the character classes or the characters themselves. Right now, whenever you want to introduce a new one, you have to search for the parts of your program that have to be changed. Of course, some things might have to be hard coded, but try to reduce that as much as possible.

Your Game class is right now doing both the logic and the output on the GUI, right? I'd separate that. Try to split up classes if they get too big.

Use smart pointers and std-containers, not raw pointers and raw arrays.

If a variable has setters and getters, that means that those should either have some logic or that you should simply make the variable public. A private variable with setters and getters has no point.

I'd focus on making the game running before adding "flesh" like the character classes etc you already devised. And if it's only some ASCII graphics screen in which you can move. Then add more and more features. Point is, there will be surprises and changes based upon you playing the game. Those might force you to do a lot of refactoring if there is already much code.

For games, there is one concept that might interest you, the paper prototype. Essentially, you take paper and stuff from board games and then create a board game that resembles your game as close as possible. Big advantage is that other people can play it. Screen for why people play it, what they like about it, what they don't, what could be easily improved. Most people have an easier time of giving advice in regard to a board game than in regard to a computer game. That said, just wanted to mention it, not a strong recommendation or anything.

\$\endgroup\$
  • \$\begingroup\$ +1. A data-driven approach is a good idea, but it might introduce too much complexity for the OP at this point, given that they're fairly new to programming. \$\endgroup\$ – Pieter Witvoet Aug 2 '17 at 11:18
  • \$\begingroup\$ Thanks for the comment, what did you mean by focus on getting the game running? I thought making the Character class was a good start? \$\endgroup\$ – user2350585 Aug 2 '17 at 16:07
  • \$\begingroup\$ @user2350585 The structures are fine. By "character class" I meant the RPG term character class, like your { HUMAN, DHUMAN, DRAGON, DREAM_WEAVER, SHINIGAMI, SOUL_EATER};, not your C++ class Character \$\endgroup\$ – Aziuth Aug 2 '17 at 19:11
  • \$\begingroup\$ "A private variable with setters and getters has no point." is exactly backward. Private members are exactly the situation where you need getters and setters, they are your only access point to that data from outside the class. \$\endgroup\$ – bluegreen Nov 15 '17 at 20:04
  • \$\begingroup\$ @bluegreen Yes, they are the only access point to the data, that is obvious. Thing is, you can do everything with the variable that you could do with a public one, by using those. If you simply made it public, the possibilities would stay the same but you had to use less code. Furthermore, the important thing here, making somewhat private is to furthen encapsulation. The reason for encapsulation is to control the state of the object. If you have free getters and setters, that is not given. Aka, it looks encapsulated but isn't. You understand the idea? \$\endgroup\$ – Aziuth Nov 16 '17 at 11:09

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.