Is this the correct way to build class hierarchy?

Question

Ok, so I have whole module that is in charge of generating player class in my game and after hours and hours of hard labour I came up with this hierarchy (snippets of code without making it too graphic but still providing enough to go on)

I have a base interface for Entities (which are either Player or Monsters):

public interface Entity {
public int getHP();
...
// all getters that denote all stats the Entities have in my game, nothing
else is in the interface
}

Then there is second interface extending Entity, called Classes, that contains all the setters relevant to classes:

public interface Classes extends  Entity {
void setHP(int a);
... //and so on}

Finally getting to some real class, class PlayerClasses is responsible for building the classes:

public class PlayerClasses implements Classes {    
private int HP, ATK, DEF, DAMAGE, DROP;

@Override
public int getHP() {
    return HP;
}

@Override
public void setHP(int a) {
    HP = a;
}

// this is how I build the player class
public void Rogue(){
   setHP(150);
   setATK(30);
   setDEF(20);
   setDAMAGE();
}

And finally a class which constructor is used to create the player instance that is then passed into other modules (no inheritance or direct access to any other class field or requiring any state in constructor, so win right?)

public class Player {
private int HP,ATK,DEF,DAMAGE,DROP;
private PlayerClasses c;

public Player() {
    c = new PlayerClasses();
    c.Rogue();
    HP = c.getHP();
    ATK = c.getATK();
    DEF = c.getDEF();
    DAMAGE = c.getDAMAGE();
    DROP = c.getDrop();
}

Kind of long question, but I tried to keep it civil. Any feedback is very appreciated.

Edit: Ok to clarify why I choose to design like this, I want the player instance to be immutable object that can only be instanced with correct values, while keeping the initialization in other modules as clean as possible without any dependencies. So for example values from two different player classes cannot mix up in case it is instances in module that shows player stats and monster stats. I feel passing private HP and ATK variables in inheritance and then pulluting namespace with same variables is not a way to go for example.

Answer 1

I think I don't understand the reason for an immutable Player class that contains a PlayerClass. But anyways, IMO your Player class is what should inherit the Entity trait. Not the PlayerClasses object that is used as sort of template(?). Because what's the point of having a Player and I assume a similarly constructed Monster class if they aren't both Entity?

You also mix responsibilites / abstractions in an odd way. What is it that is encapsulated in the PlayerClasses and in the Player? PlayerClasses looks like it should represent the class type like "Rogue", not the actual player. And for that it shouldn't have setter methods and neither is a class type an entity. And a factory like method that initializes a PlayerClasses object is "bad" style. You should always try to guarantee based only on class type that things are right, not have magic methods that need to be called for objects to be right (i.e. no init methods besides the constructor).

Take for example a method that takes a PlayerClasses object as parameter and you want someone else to use that code. They see that they need a reference to PlayerClass and a no-argument constructor for that class, but they can't know what all the initialization steps are. Constructor or various factory / builder patterns can guarantee exactly that.

Here's a draft of how I would have done it:

interface PlayerClass {
    int getBaseHp();
    int getBaseAtk();
    ... // more class attributes
}

// as utility so I don't have to write 20 full classes
abstract class PlayerClassBase implements PlayerClass {
    private final int hp, atk, ..;
    protected PlayerClassBase(int hp, int atk, ...) {
        this.hp = hp;
        this.atk = atk;
    }
    @Override
    public int getBaseHp() {
        return hp;
    }
    ....
}

// short class but guaranteed that every instance of Rogue has the correct values
class Rogue {
     public Rogue() {
         super(40, 23, 123, ...);
     }
}

// something to represent entities
interface Entity {
    int getCurrentHp();
    int takeDamageFrom(Entity other);
    ...
}

// maybe an abstract base class here as well

// represents a player that has an immutable class and it can't exist without
class Player implements Entity {
    privte final PlayerClass playerClass;
    private int currentHp;
    ...

    public Player(PlayerClass playerClass) {
        this.playerClass = playerClass;
        currentHp = playerClass.getHp();
        ...
    }
    public int takeDamageFrom(Entity other) {
        currentHp -= other.getCurrentAtk();
        return currentHp;
    }

}

The PlayerClass part could also be a simple enum instead of a big class hierarchy.

enum PlayerClass {
    ROGUE(23, 23, 4, ...),
    ...
    ;
    private final int hp;
    PlayerClass(int hp, int atk, ...) {
        this.hp = hp;
        ...
    }
    public int getHp() { return hp; }
    ...
}

That way you could statically reference PlayerClass.ROGUE and create a player like this: new Player(PlayerClass.ROGUE). Instead of currently new Player(new PlayerClass().Rogue()). Or with the big hierarchy: new Player(new Rogue())