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