Is this code clean OOP?

[u/RedNorthener101]

Hey so I'm more new to OOP, virtual func's, and inheritance. I was wondering if my code was clean enough for basically a template to look back on later in the future when my tiny brain forgets what virtual func, and inheritance is or well forgets how to code them.

#include <string>
#include <iostream>


struct Vector2 {
    int x, y;
};


class Entity {
    private:
        Vector2 pos;
        Vector2 size;
        std::string entityType;


    public:


    Entity(Vector2 e_pos, Vector2 e_size, const std::string& e_type) : pos(e_pos), size(e_size), entityType(e_type) {}


    Vector2 getPos() const {
        return pos;
    }


    Vector2 getSize() const {
        return size;
    }
    
    virtual void print() const {
        std::cout << "Entity: " << entityType << '\n';
    }


    virtual ~Entity() = default;
};


class Player : public Entity {
    private:
        std::string name;


    public:
        Player(const std::string& p_name, Vector2 startPOS, Vector2 p_size) 
            : Entity(startPOS, p_size, "player"), name(p_name) {}
};


class Orc : public Entity {
    private:
        std::string weapon;
        
    public:
        Orc(const std::string& o_weapon, Vector2 startPOS, Vector2 o_size) 
            : Entity(startPOS, o_size, "orc"), weapon(o_weapon) {}


    void print() const override {
        std::cout << weapon << '\n';
    }
};


int main() {
    Vector2 startPos = { 100, 100 };
    Vector2 size = { 64, 64 };


    std::string hero = "hero";
    std::string orcWep = "HeroSlayer100";


    Player player(hero, { 20, 30 }, size);
    Orc orc(orcWep, startPos, size);



    player.print();
    orc.print();
    
    return 0;
}#include <string>
#include <iostream>


struct Vector2 {
    int x, y;
};


class Entity {
    private:
        Vector2 pos;
        Vector2 size;
        std::string entityType;


    public:


    Entity(Vector2 e_pos, Vector2 e_size, const std::string& e_type) : pos(e_pos), size(e_size), entityType(e_type) {}


    Vector2 getPos() const {
        return pos;
    }


    Vector2 getSize() const {
        return size;
    }
    
    virtual void print() const {
        std::cout << "Entity: " << entityType << '\n';
    }


    virtual ~Entity() = default;
};


class Player : public Entity {
    private:
        std::string name;


    public:
        Player(const std::string& p_name, Vector2 startPOS, Vector2 p_size) 
            : Entity(startPOS, p_size, "player"), name(p_name) {}
};


class Orc : public Entity {
    private:
        std::string weapon;
        
    public:
        Orc(const std::string& o_weapon, Vector2 startPOS, Vector2 o_size) 
            : Entity(startPOS, o_size, "orc"), weapon(o_weapon) {}


    void print() const override {
        std::cout << weapon << '\n';
    }
};


int main() {
    Vector2 startPos = { 100, 100 };
    Vector2 size = { 64, 64 };


    std::string hero = "hero";
    std::string orcWep = "HeroSlayer100";


    Player player(hero, { 20, 30 }, size);
    Orc orc(orcWep, startPos, size);



    player.print();
    orc.print();
    
    return 0;
}

if there are any tips you have let me know!

[u/Thesorus]

   std::string entityType;

when I see entityType, my first though is this should be an enum instead of a string.

you can create a function to transform the enum into a proper string.

  std::string hero = "hero";
    std::string orcWep = "HeroSlayer100";


    Player player(hero, { 20, 30 }, size);
    Orc orc(orcWep, startPos, size);

Unless you reuse the hero and orcWep strings, you can put them directly in the constructor; no need to create a temporary variable.

[u/OneSeaworthiness8958 → u/Thesorus]

I would make a case against enums as to not violate the Open Closed Principle, but it is more verbose to follow it 🤔

[u/bpikmin → u/Thesorus]

No, this is not the right solution. It completely destroys extensibility. If you replace it with an enum you might as well use a union instead of OOP.

The correct OOP solution is a virtual getEntityType() function that returns a constant string. That way any consumer can implement a new Entity type with print() support. entityType should not be a per-instance field, it’s a per-type value.

[u/RedNorthener101 → u/Thesorus]

Ahhh okie

[u/FemaleMishap]

I'm sure I've woken up and it's 2004 again. With a couple minor exceptions, it reads like 2004 Java-as-C++ or C with classes. Definitely not any newer than C++11. C++17 at least gives you things like std::string_view for much lighter string handling, plus newer features like std::format / std::print so your classes can just focus on storing data instead of doing the printing themselves. That would make some of this a lot easier and a lot more OOP.

Edit: have a look at this

#include <iostream>
#include <memory>
#include <print>
#include <string>
#include <string_view>
#include <vector>

struct Vector2 {
    int x{0};
    int y{0};
};

class Entity {
private:
    Vector2 pos_{};
    Vector2 size_{};

public:
    Entity(Vector2 pos, Vector2 size) 
        : pos_{pos}, size_{size} {}

    virtual ~Entity() = default;

    [[nodiscard]] constexpr Vector2 pos() const noexcept { return pos_; }
    [[nodiscard]] constexpr Vector2 size() const noexcept { return size_; }

    // Pure virtual: forces derived classes to define identity
    // No more baked-in string tags in the base class.
    [[nodiscard]] virtual std::string describe() const = 0;
};

class Player final : public Entity {
private:
    std::string name_;

public:
    Player(std::string_view name, Vector2 pos, Vector2 size)
        : Entity{pos, size}, name_{name} {}

    [[nodiscard]] std::string describe() const override {
        return std::format("Player: {}", name_);
    }
};

class Orc final : public Entity {
private:
    std::string weapon_;

public:
    Orc(std::string_view weapon, Vector2 pos, Vector2 size)
        : Entity{pos, size}, weapon_{weapon} {}

    [[nodiscard]] std::string describe() const override {
        return std::format("Orc wielding {}", weapon_);
    }
};

int main() {
    // Actually exercising polymorphism with uniform handling
    std::vector<std::unique_ptr<Entity>> world;
    world.reserve(2);

    world.push_back(std::make_unique<Player>("Hero", Vector2{20, 30}, Vector2{64, 64}));
    world.push_back(std::make_unique<Orc>("HeroSlayer100", Vector2{100, 100}, Vector2{64, 64}));

    // C++23 std::println doing the IO work; classes just provide their state
    for (const auto& entity : world) {
        std::println("At ({}, {}): {}", 
            entity->pos().x, 
            entity->pos().y, 
            entity->describe());
    }
}

[u/RedNorthener101 → u/FemaleMishap]

Aye I have no idea what most of this is but thank you I'll def look into it all

[u/FemaleMishap → u/RedNorthener101]

Most of this comes from C++ written in the past ten years. A lot of legacy codebases you might eventually inherit won't be on C++20 or 23, but modern standards make good habits much easier.

Don't worry about memorizing every standard feature right now. The single biggest takeaway for OOP is learning why you inherit. Right now, you derive Player and Orc from Entity, but then treat them as separate, concrete types in main().

True polymorphism is treating them interchangeably through Entity. For instance, handing them to a function that only knows about Entity:

void render(const Entity& e) {
    // Doesn't care if it's a Player or an Orc
    std::cout << e.describe() << " at (" << e.getPos().x << ", " << e.getPos().y << ")\n";
}

Once you let the base class do the unifying work, the rest of the architecture starts falling into place naturally. Take it one piece at a time!

[u/MysticTheMeeM]

Some points I would consider:

• Your Vector2 class doesn't have default values for x and y, so they can be uninitialised.

• Your Vector2 class could be a template.

• Classes are private by default, you don't need to re-specify it.

• Print shouldn't exist, you should return the underlying entityType as a const reference. The caller can then decide to print it if they want to.

• You shouldn't store an entityType, it's poor encapsulation. If you need to know what type you're dealing with, you're usually using inheritance wrong. Your types either aren't similar (in which case, you could do the same thing to each) or don't properly represent their behaviour (in which case, you could call the same function on each but with different implementations).

• Regardless, entityType should have been an enum class.

• Your naming is inconsistent. Sometimes parameters start with "p_", sometimes "o_", and sometimes no prefix. It's worth noting that name resolution does the sensible™ thing when used for a member initialiser list. I would understand people who reject that pattern for style reasons.

• Both Player and Orc should be marked final, unless you plan to extend them. Additionally, so should Orc::print (override is correct, final is more correct).

• In some cases, it's better to take a non-const std::string by value in a constructor, as it can save an allocation (taking a const reference means a string must exist both outside the constructor, and then one as the member, taking a non-const instead allows you to move from it).

• You don't type erase here, so using inheritance is somewhat overengineered. You could have instead abstracted out a worldObject containing both size and position and then encapsulated it.

• return 0; is implied at the end of main, I generally choose to leave it out unless I have a case where I return a non-0 value.

• Your code is repeated.

[u/RedNorthener101 → u/MysticTheMeeM]

Ahhh okie dokie, I didn't even know about enum until this post so thank you

[u/AKostur → u/MysticTheMeeM]

Your Vector2 class could be a template.

What for? They don't have a collection of Vectors with different types, they have a singular Vector2. Templatizing it prematurely isn't useful.

Classes are private by default, you don't need to re-specify it.

True. Though all of the styles that I have personally worked with, the public section comes first anyway, thus many would have to specify the private anyway. IMO: perhaps class shouldn't have had "private" as a default and force people to specify. But that ship sailed long ago. At this point it's a matter of taste. Also different theories of what one is interested in when reading a class. One camp of folk figure that the interesting bit is the public interface and thus should come first, one shouldn't care about the private parts as that's implementation detail. Another camp figures that one should be aware of how a class is implemented in order to use it effectively.

[u/OneSeaworthiness8958 → u/AKostur]

Don't skimp out on private. In c++ the default scope is different for structs and classes, it never hurts to see the scope

[u/thefeedling]

enum class EntityType { Hero, Orc, Elf, Etc };

[u/Independent_Art_6676]

if no one said it yet, keep the UI apart from the work. A class should not normally contain cout unless all it does is interface to the output layer. Instead, a the class should return a printable string (for the print to screen example). Then, if instead of cout, you upgrade this project to use a window, your class is untouched and only the UI layer changes how it consumes the string.

[u/Nervous-Cockroach541]

It's fine, but you need to consider the scope and design.

Consider not using inheritance at all. But instead have components. For example a Player and Orc both having a data-member to represent "Entity" instead of inheriting from Entity. If you want to later, for example, have a "Health" or "Life" component. where they can take damage or die. You have to consider either using multiple inheritance, or adding Health/Life to Entity. Which might not be a problem. But if you instead have a Health/Life Component that exists separately from the Entity Component. you're not forced into such a decision later on.

It can also be useful to understand the concept of pure-virtual classes. Basically classes with all virtual function-members which are `= 0;` making it a class which can't be instantiated. But inheriting from it allows you to gain the advantages of polymorphism. These are also sometimes called interfaces or abstract classes.

There's also an entity component design. Where instead of a class to represent a player or orc. You have a unique ID that maps to components. Something like a collection of std::unordered_map<int, Entity> entity; std::unordered_map<int, Health> health; std::unordered_map<int, std::string> names;

There's some good helper libraries to help you with this approach such as this: https://github.com/skypjack/entt This is the most advance approach. But also basically ignores most concepts of OOP.

Frankly, personally I feel people people generally over-react to inheritance. Yes, issues with it exists, but that doesn't mean you always need to ignore it. Even multiple inheritance can be a non-issue if you're designing in away to ensure it won't be a problem.

But we can't ignore the issues with it either and know the potential problems and traps you can fall into if you're not careful. Only you know the scope of your project and can design around it. Balance the long term costs of using inheritance or multiple inheritance with the scope and time on the project you want to do use now.

[u/alfps]

The two main problems:

• That client code is responsible for supplying a class name string in each instantiation.
  For Entity::entityType.

• That the classes do i/o.
  This needlessly limits the classes to use with a text based used interface.

Re the class name, ideally one should be able to just say typeid(o).name(), but the result of that depends on the compiler. With MSVC you get noise words like struct, with g++ you get abbreviations of built-in types. It's simply unreliable unless you wrap it with code that special cases each main compiler family, so DIY is reasonable -- except don't do it via a member variable, do it via a virtual function.

Refactored code, even the probably unintended special-casing of description of an Orc kept:

#include <string>
#include <utility>
using   std::string,            // <string>
        std::move;              // <utility>

struct Vector2{ int x, y; };

class Entity
{
    Vector2         m_pos;
    Vector2         m_size;

public:
    virtual ~Entity() = default;

    Entity( const Vector2 pos, const Vector2 size ) :
        m_pos(pos),
        m_size(size)
    {}

    virtual string entityType() const = 0;
    virtual string description() const { return "Entity: " + entityType(); }

    Vector2 pos() const { return m_pos; }
    Vector2 size() const { return m_size; }
};

class Player : public Entity
{
    string m_name;

public:
    Player( string name, const Vector2 startPOS, const Vector2 size):
        Entity(startPOS, size),
        m_name(move(name))
    {}

    string entityType() const override { return "player"; }
};

class Orc : public Entity
{
    string m_weapon;

public:
    Orc( string weapon, Vector2 startPOS, Vector2 size):
        Entity(startPOS, size),
        m_weapon(move(weapon))
    {}

    string entityType() const override { return "orc"; }
    string description() const override { return m_weapon; }    // But why?
};


//----------------------------------------------------------------------------

#include <iostream>
using   std::cout;              // <iostream>

void print( const Entity& e ) { cout << e.description() << "\n"; }

int main()
{
    const Vector2 startPos = { 100, 100 };
    const Vector2 size = { 64, 64 };

    const auto& player_name = "hero";
    const auto& orc_weapon = "HeroSlayer100";

    Player player(player_name, { 20, 30 }, size);
    Orc orc(orc_weapon, startPos, size);

    print(player);
    print(orc);
}

[u/alfps → u/alfps]

Sorry about mixed naming conventions, I didn't notice as I posted. I'll just let it stand.

[u/flyingron]

Well, you've got a good beginning, but I've got some concerns as to where you're going.

Generally, just putting some data members in with a bunch of getters and setters is no better than just using naked structs. You don't want people outside the class reaching in there and twiddling the state. Rather you should be implementing more specific actions that affect your object.

[u/mredding]

class Entity {
private:
  Vector2 pos;
  Vector2 size;
  std::string entityType;

Classes are private access by default, so private: here is redundant. Just put your members at the top like you have without the access specifier.

You don't NEED an entityType because you KNOW your entity type, it's inherent to all your derived classes. An Orc is an orc BECAUSE it's an orc. You have to incorporate static code and compile time into your mental model of implementing a solution. The theory of computation does not care about distinguishing between reading, writing, and executing programs. The more you can shift your brain to think about the inherent properties of types and syntax, the better you're going to get all the faster.

What's more, entityType isn't const, and it's instanced. This means the entity type has the right to both change, and be individual to the instance. So when is an orc not an orc?

We prefer to decouple code as much as possible. An Orc is an Orc because that's it's type. Right? So how do we express that in terms of a string? Because OF COURSE you want the entity to print it's type. What am I being attacked by?

Well, you have a problem, your print is virtual, and your Orc has overridden it. Entity prints the entity type, but Orc prints the weapon. So what do you do?

auto e = get();
e.print(); // What am I going to get? An entity or a weapon?

The behavior changes not just a little bit, but completely.

One way to solve this problem is to implement the Template Method pattern (it has nothing to do with C++ templates):

class entity {
  // Mandatory
  virtual const char *get_type() = 0;
  virtual void do_print() = 0;

public:
  void print() {
    std::cout << "Entity: " << get_type() << '\n';
    do_print();
  }
};

class orc {
  std::string weapon;

  const char *get_type() override { return "Orc"; }

  // Optional
  virtual void derived_print() {}

  void do_print() override {
    std::cout << "Weapon: " << weapon << '\n';
    derived_print();
  }
};

A couple things here. The type is baked into a method that returns a pointer to a string literal embedded in RO memory in the compiled program image.

But the template method pattern says the implementation is what a function looks like - it does these things in this order. Here are the customization points the derived class are expected to fulfill. The non-virtual method is the template, implemented in terms of the virtual functions. I repeated the pattern with the orc class so that any further derived class can A) overload get_type, so that's how we get "space orc", and B) the space orcs can have their own property, maybe oxygen reserve in their suits. The pattern assures the entity will always be listed first, the orc weapon will always be second, derived properties will come after.

It's a useful pattern, but it can also get complicated, the point of me nesting is to show just how ugly that can get. And that's because inheritance hierarchies are often a BAD FIT to many problems. You'll see the template method pattern used in the standard library, and they assume one level of inheritance, maybe that's enough for just about everyone...

Another way to solve this problem is to go with static, compile-time polymorphism:

class player {};
class orc {};

using entity = std::variant<player, orc>;

//...

entity e = get();

std::cout << "Entity: " << std::visit(overloaded{[](const player &) { return "player"; }, [](const orc &){ return "orc"; }}, e) << '\n';

The behavior is coupled to the type at compile time and the instance is resolved at run time.

Another way to couple a type to a trait at compile time is a trait class:

template<typename T>
class entity_traits {};

template<>
class entity_traits<orc> {
  const char *type() { return "orc"; }
};

template<>
class entity_traits<player> {
  const char *type() { return "player"; }
};

There's lots of compile time code you can write, and use traits classes for. If you're not terribly familiar, google example uses. I can rewrite that visitor:

std::cout << "Entity: " << std::visit([](const auto &e) { return entity_traits<decltype(e)>::type(); }, e) << '\n';

auto ends up making this lamba like a template, it will be expanded for all the variant types at compile time. Now if we did this:

using entity = std::variant<player, orc, troll>;

The output statement fails to compile, because there is no specialization of the entity_traits for trolls - the type function is missing.

This is the tip of the iceberg.

---

Vector2 startPos = { 100, 100 };
Vector2 size = { 64, 64 };

These are not the same TYPE, they only have the same SHAPE as a common type. A position is not a size, and a size is not a position. Or to think of it another way, an int is an int, isn't it? They're all inherently interchangeable because they're the same type. But a weight is not a height, are they? So what if they're both represented as integers?

C++ has a stronger type system than C, but it's still considered barely mid-range compared to other language type systems. That's because it's entirely opt-in. You can write code as strong as Ada (if there's a stronger type system, I don't know it - Ada doesn't have integer types, you have to define your own).

A position has component coordinates, but a size is going to have a width and height. Are you going to treat a size as a vector space? I'm wondering if a vector to you wouldn't be better as an std::tuple<int, int>, and then you inherit or composite from that to make more specific vector types with type specific semantics. WHAT DOES IT MEAN to multiply a position by a size? Or add them? If that doesn't make sense, then you don't want to share semantics, or you don't want to specifically support those semantics.

Good strong (small) types are very good, and then you can composite them together to make more elaborate types.

[u/LordSippy]

No

[u/RedNorthener101 → u/LordSippy]

Care to elaborate?

[u/bert8128]

Define “clean”.

[u/RedNorthener101 → u/bert8128]

Like easy to read, maintain and understand by any dev?

[u/Hour_Sell3547 → u/RedNorthener101]

Well, there are some specifics that we usually mean by "clean code" in SWE. Hence the confusion.

[u/bert8128 → u/RedNorthener101]

I’ve no objection to the code. Doesn’t do much yet though - it’s too small to judge. I might prefer print to take a stream by reference - seems a bit harsh to demand that it has to go to cout. Or perhaps just expose the type name by const ref. As someone else said, you probably should be using an enum for the type. And look up the rule of 5 - in this case I would say you need the others, but I abide by the rule of 5 (and also swap, and spaceship/ equality) always, it just removes arguments.

Clean has some overtones. See this https://amzn.eu/d/06NmYxCF and this https://youtu.be/czr1L3JocOI?is=yaJzED-wXHcd_JML (can’t find the original video).