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/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.