Player class for RPG/DND type game

Question

I am recreating a game that I used to play on the Nintendo DSI, 'Dragon Quest IX'. I've made a player class, but have run into a code efficiency issue. When the Player object is first instantiated, it has a constructor parameter player_class that determines what class the user has chosen. In the method setAttributes, it determines the baseline stats for the newly created player. I use multiple if statements to achieve this. Is there any way I can make this more compact/efficient, without having to have big blocky if statements that generically contain all the same code? Any and all help/suggestions are appreciated and considered.

Player.java

/*
* Player Class
*/
public class Player {

    /*
    * Private Instance Variables
    */
    private String name;
    private String player_class;
    private int experience;
    private int level;
    private int gold;
    private int hp;
    private int mp;
    private int strength;
    private int agility;
    private int resilience;
    private int deftness;
    private int charm;
    private int magical_mending;
    private int magical_might;

    /*
    * Bag object instantiation
    */
    private Bag b;

    /*
    * New Player Constructor (no default since name is needed)
    */
    public Player(String name, String player_class) {
        this.name = name;
        this.player_class = player_class;
        this.experience = 0;
        this.level = 0;
        this.gold = 0;
        this.b = new Bag();
    }

    /*
    * Determine stats based on class chosen 
    */
    private void setAttributes() {
        /*
        * Can be optimized later, if statements will do
        */
        if(this.player_class.equals("Warrior")) { /* DONE */
            this.hp = 26;
            this.mp = 4;
            this.strength = 18;
            this.agility = 4;
            this.resilience = 18;
            this.deftness = 5;
            this.charm = 4;
            this.magical_mending = 0;
            this.magical_might = 0;
        }
        if(this.player_class.equals("Priest")) { /* DONE */
            this.hp = 19;
            this.mp = 14;
            this.strength = 9;
            this.agility = 14;
            this.resilience = 9;
            this.deftness = 9;
            this.charm = 7;
            this.magical_mending = 18;
            this.magical_might = 0;
        }
        if(this.player_class.equals("Mage")) { /* DONE */
            this.hp = 18;
            this.mp = 16;
            this.strength = 4;
            this.agility = 18;
            this.resilience = 7;
            this.deftness = 14;
            this.charm = 7;
            this.magical_mending = 0;
            this.magical_might = 18;
        }
        if(this.player_class.equals("Martial Artist")) { /* DONE */
            this.hp = 24;
            this.mp = 2;
            this.strength = 18;
            this.agility = 23;
            this.resilience = 11;
            this.deftness = 11;
            this.charm = 5;
            this.magical_mending = 0;
            this.magical_might = 0;
        }
        if(this.player_class.equals("Thief")) { /* DONE */
            this.hp = 23;
            this.mp = 6;
            this.strength = 13;
            this.agility = 18;
            this.resilience = 11;
            this.deftness = 18;
            this.charm = 3;
            this.magical_mending = 4;
            this.magical_might = 0;
        }
        if(this.player_class.equals("Minstrel")) { /* DONE */
            this.hp = 20;
            this.mp = 6;
            this.strength = 9;
            this.agility = 8;
            this.resilience = 8;
            this.deftness = 12;
            this.charm = 9;
            this.magical_mending = 7;
            this.magical_might = 6;
        }
        if(this.player_class.equals("Gladiator")) { /* DONE */
            this.hp = 32;
            this.mp = 2;
            this.strength = 30;
            this.agility = 7;
            this.resilience = 19;
            this.deftness = 15;
            this.charm = 5;
            this.magical_mending = 0;
            this.magical_might = 0;
        }
        if(this.player_class.equals("Armamentalist")) { /* DONE */
            this.hp = 32;
            this.mp = 14;
            this.strength = 21;
            this.agility = 10;
            this.resilience = 15;
            this.deftness = 7;
            this.charm = 11;
            this.magical_mending = 0;
            this.magical_might = 17;
        }
        if(this.player_class.equals("Paladin")) { /* DONE */
            this.hp = 36;
            this.mp = 11;
            this.strength = 21;
            this.agility = 4;
            this.resilience = 22;
            this.deftness = 1;
            this.charm = 7;
            this.magical_mending = 10;
            this.magical_might = 0;
        }
        if(this.player_class.equals("Ranger")) { /* DONE */
            this.hp = 31;
            this.mp = 14;
            this.strength = 18;
            this.agility = 16;
            this.resilience = 14;
            this.deftness = 30;
            this.charm = 4;
            this.magical_mending = 12;
            this.magical_might = 0;
        }
        if(this.player_class.equals("Sage")) { /* DONE */
            this.hp = 29;
            this.mp = 30;
            this.strength = 12;
            this.agility = 13;
            this.resilience = 12;
            this.deftness = 3;
            this.charm = 14;
            this.magical_mending = 12;
            this.magical_might = 14;
        }
        if(this.player_class.equals("Luminary")) { /* DONE */
            this.hp = 31;
            this.mp = 13;
            this.strength = 9;
            this.agility = 22;
            this.resilience = 12;
            this.deftness = 16;
            this.charm = 18;
            this.magical_mending = 19;
            this.magical_might = 5;
        }
    }

    /*
    * levelUp method for increasing stats of player
    */
    public void levelUp() {
        this.hp += this.level + 5;
        this.mp += this.level + 2;
        this.strength += this.level + 2;
        this.agility += this.level + 2;
        this.resilience += this.level + 1;
        this.deftness += this.level + 2;
        this.charm += this.level + 1;
        this.magical_mending += this.level * 2;
        this.magical_might += this.level * 2;
        this.level += 1;
    }

    /*
    * sellItem, sells item for value
    */
    public void sellItem(Item[] item) {
        this.gold += item.getValue();
        for(int i = 0; i <= this.bag.length - 1; i++) {
            if(this.bag[i].getName().equals(item.getName())) {
                this.bag[i] == null;
                break;
            }
        }
    }

    /*
    * buyItem, used to add item from vendor
    */
    public void buyItem(Item[] item) {
        if(this.gold >= item.getValue()) {
            this.gold -= item.getValue();
            for(int i = 0; i <= this.bag.length - 1; i++) {
                if(!(this.bag[i] == null)) {
                    this.bag[i] == item;
                    break;
                }
            }
        } else {
            System.out.println("Cannot Afford Item!");
        }
    }

    /*
    * addItem, used for adding items from monster drops
    */
    public void addItem(Item[] item) {
        for(int i = 0; i <= this.bag.length - 1; i++) {
            if(!(this.bag[i] == null)) {
                this.bag[i] == item;
                break;
            }
        }
    }

    /*
    * useItem, for items in medical class
    */
    public void useItem(Medical item) {
        this.hp += item.getHealValue();
        for(int i = 0; i <= this.bag.length - 1; i++) {
            if(this.bag[i].getName().equals(item.getName())) {
                this.bag[i] == null;
                break;
            }
        }
    }

    /*
    * isDead method, returns a boolean if current health is less than/equal to 0
    */
    public boolean isDead() {
        return this.hp <= 0;
    }

    /*
    * Getters for player class below
    */
    public int getLevel() {
        return this.level;
    }
    public int getGold() {
        return this.gold;
    }

}

tester.java

/*
* Tester class for Enemy/Player
*/
public class tester {

    /*
    * Main Method
    */
    public static void main(String[] args) {

        Player player = new Player("Ben", "Warrior");

    }

}

Answer 1

First, I would not use Strings here to represent class types. Say you accidentally typo a class name:

new Player("Hero", "Theif") // Whoops

Now setAttributes silently "fails" and doesn't set anything. You could manually handle bad cases like this, but now you're handling bad data at runtime that could have been caught at compile time.

If you have a limited set of options, create an enum:

public enum PlayerClass {
    WARRIOR, PRIEST, MAGE, ... 
}

Then use it like:

private PlayerClass player_class;

. . .

if(this.player_class == WARRIOR)) { ...

The immediate benefits are IDEs can help auto-complete enum names so typos are difficult to cause, and, if you do typo a name, it will fail with an error at compile time instead of having code-dependent effects at runtime.

This doesn't answer your main question, but it's an important point. Don't use Strings to mark "members of a set", like you're using in this case to mark members of the player class set.

---

I'd refactor your class a bit to make dealing with class stats easier. I think Player is too big, with too many fields. I would create a Stats class:

class Stats {
    public int hp;
    public int mp;
    public int strength;
    public int agility;
    public int resilience;
    public int deftness;
    public int charm;
    public int magical_mending;
    public int magical_might;

    // Copy constructor to make copying stats easier
    public Stats(Stats other) {
        this(other.hp, other.mp,
             other.strength, other.agility, other.resilience, other.deftness,
             other.charm, other.magical_mending, other.magical_might);
    }

    // This part sucks, but it's necessary in POD classes
    public Stats(int hp, int mp,
                 int strength, int agility, int resilience, int deftness,
                 int charm, int magical_mending, int magical_might) {

        this.hp = hp;
        this.mp = mp;
        this.strength = strength;
        this.agility = agility;
        this.resilience = resilience;
        this.deftness = deftness;
        this.charm = charm;
        this.magical_mending = magical_mending;
        this.magical_might = magical_might;
    }
}

Then create a mapping between class types and instances of stats:

// "of" requires Java 9
Map<PlayerClass, Stats> classToStats = Map.of(
    PlayerClass.WARRIOR, new Stats(26, 4, 18, 4, 18, 5, 4, 0, 0),
    PlayerClass.PRIEST, new Stats(19, 14, 9, 14, 9, 9, 7, 18, 0)
    // ...
);

And use it in the player constructor:

public class Player {
    private String name;
    private PlayerClass playerClass;
    private Stats stats;

    public Player(String name, PlayerClass playerClass) {
        this.name = name;
        this.playerClass = playerClass;

        // Using the copy constructor of Stats to prevent multiple players getting the same mutable stat object
        this.stats = new Stats(classToStats.get(playerClass));
    }

}

There's going to be bulk somewhere that decides what stats each class have. You just need to find how you can organize the bulk so it's readable and easier to deal with.

The main issue with this design choice is hp and mp are now a part of Stats, which feels a little odd since these are "fluid" values that can change often. Now to damage the player, you need to alter player.stats.hp. It may be better to change hp and mp in Stats to maxHp and maxMP (since you'd likely need to track those values anyways), then give Player back hp and mp fields that you alter as needed.

---

And just a note, Java uses camelCase, not snake_case.

Answer 2

First of all, mine is an addition to Carcigenicate's answer. This is taking some of the concepts one step further and refining them using design patterns

// This part sucks, but it's necessary in POD classes
public Stats(int hp, int mp,
             int strength, int agility, int resilience, int deftness,
             int charm, int magical_mending, int magical_might) {

    this.hp = hp;
    this.mp = mp;
    this.strength = strength;
    this.agility = agility;
    this.resilience = resilience;
    this.deftness = deftness;
    this.charm = charm;
    this.magical_mending = magical_mending;
    this.magical_might = magical_might;
}

I agree with the comment - this does suck. The problem here is remembering what is what, when we do new Stats(1, 2, 3, 4, 5, 6, 7, 8, 9) it's very hard to remember what 6 is or if that's even the correct number of arguments. I even had to check three times to make sure it was correct and even then I found I had miscounted. To solve this problem, we can use the Builder pattern:

public class StatsBuilder {
    private int hp;
    private int mp;
    private int strength;
    private int agility;
    private int resilience;
    private int deftness;
    private int charm;
    private int magicalMending;
    private int magicalMight;

    public StatsBuilder setHp(int hp) {
        this.hp = hp;
        return this;
    }

    public StatsBuilder setMp(int mp) {
        this.mp = mp;
        return this;
    }

    public StatsBuilder setStrength(int strength) {
        this.strength = strength;
        return this;
    }

    public StatsBuilder setAgility(int agility) {
        this.agility = agility;
        return this;
    }

    public StatsBuilder setResilience(int resilience) {
        this.resilience = resilience;
        return this;
    }

    public StatsBuilder setDeftness(int deftness) {
        this.deftness = deftness;
        return this;
    }

    public StatsBuilder setCharm(int charm) {
        this.charm = charm;
        return this;
    }

    public StatsBuilder setMagicalMending(int magicalMending) {
        this.magicalMending = magicalMending;
        return this;
    }

    public StatsBuilder setMagicalMight(int magicalMight) {
        this.magicalMight = magicalMight;
        return this;
    }

    public Stats build() {
        return new Stats(hp, mp, strength, agility, resilience, deftness, charm, magicalMending, magicalMight);
    }
}

Note: a lot of IDEs have support to generate this for you via refactoring tools. They can even replace any direct calls to the constructor with using the Builder.

Since it a fluent interface we can chain the calls, so

new Stats(26, 4, 18, 4, 18, 5, 4, 0, 0)

turns into the much more descriptive, albeit longer

new StatsBuilder()
        .setHp(26)
        .setMp(4)
        .setStrength(18)
        .setAgility(4)
        .setResilience(18)
        .setDeftness(5)
        .setCharm(4)
        .setMagicalMending(0)
        .setMagicalMight(0)
    .build()

We now know exactly what you're setting to what without needing to look up the values. We can even set these in any order you want.

The Builder can also make copies of the stats for you if you just define a method Stats buildFrom(Stats otherStats) that takes a Stats object and creates a new one copying each property. But I'm just mentioning it, you might opt for a different route.

---

In addition to this, as a further step from removing the stats logic from the Player class, we can also remove the level up logic by using a delegation

Here is what this can look like. First, we'll separate the levelling functionality:

public class Leveller {
    public void levelUp(Player player) {
        player.getStats().setHp(player.getStats().getHP() + player.getLevel() + 5);
        player.getStats().setMp(player.getStats().getMp() + player.getLevel() + 2);
        player.getStats().setStrength(player.getStats().getStrength() + player.getLevel() + 2);
        player.getStats().setAgility(player.getStats().getAgility() + player.getLevel() + 2);
        player.getStats().setResilience(player.getStats().getResilience() + player.getLevel() + 1);
        player.getStats().setDeftness(player.getStats().getDeftness() + player.getLevel() + 2);
        player.getStats().setCharm(player.getStats().getCharm() + player.getLevel() + 1);
        player.getStats().setMagicalMending(player.getStats().getMagicalMending() + player.getLevel() * 2);
        player.getStats().setMagicalMight(player.getStats().getMagicalMight() + player.getLevel() * 2);

        player.setLevel(player.getLevel() + 1);
    }
}

This is verbose but can be made shorter. For example, if you implement addX methods, that does addX(int newX) { this.x += newX } for each of the attributes and maybe if you also just pass in Stats and level, so you don't have to do .getStats() and .getLevel() all the time. These are options - it's your choice. This an implementation only utilizes get and set methods for illustrative purposes.

Now, we can have the Leveller handle any levelups, thus removing that logic from the Player class. You can implement this like so:

public class Player {
    // other private variables
    //. . .
    // /other private variables

    private Leveller leveller = new Leveller();

    // other methods
    // . . .
    // /other methods

    public void levelUp() {
        leveller.levelUp(this);
    }
}

You can even make the Leveller a private static final variable, as it's not going to change at runtime, nor do you need multiple of these objects - a single Leveller can handle any player.

---

However, taking this out leads to something interesting we can do. But to get there, first let's take a look at Java enums - the most important thing about them is that unlike other languages, Java enums are entire classes by themselves and can have methods and variables. In our case, this means we can associate classes more strongly with their stats by integrating them into the enum:

public enum PlayerClass {
    WARRIOR, PRIEST, MAGE, ... 
}

and

Map<PlayerClass, Stats> classToStats = Map.of(
    PlayerClass.WARRIOR, new Stats(26, 4, 18, 4, 18, 5, 4, 0, 0),
    PlayerClass.PRIEST, new Stats(19, 14, 9, 14, 9, 9, 7, 18, 0)
    // ...
);

can be more explicitly bound together by in this:

public enum PlayerClass {

    WARRIOR(new Stats(26, 4, 18, 4, 18, 5, 4, 0, 0)), //<-- we are passing new Stats(/*...*/) into the constructor
    PRIEST(new Stats(19, 14, 9, 14, 9, 9, 7, 18, 0)),
    //...
    ; //<-- the semi-colon is needed if you want to define variables, methods, or a constructor

    private final Stats startingStats;

    PlayerClass(Stats startingStats) {
        this.startingStats = startingStats;
    }

    public Stats getStartingStats() {
        return startingStats; //it might be easier you can return a copy here but I'm keeping the code simple
    }
}

You can, of course, use the StatsBuilder to be more clear. I opted for conciseness and closeness to the previous example.

At any rate, now we have a more explicitly bound stats to classes because they really aren't separable - you can't have the warrior without the warrior's stats or vice versa. When we define them in the same place we enforce that binding and make it clear how it works. Otherwise if you come back to this code in a year, you might struggle to find where the connection between class and stats was.

---

So, let's now come back to Leveller and what is the interesting thing to do here. We can have a Leveller as a variable defined in the PlayerClass enum. It doesn't make sense right now as there is a single level up mechanic defined, however, it might if we want a separate ones that can vary per class. For now, let's make it simple - we'll define a MagicLeveller and MightLeveller that will increase the stats for mages and fighters.

Here, we can use the Template Method design pattern. First we can define Leveller to be an abstract class - we always want to increase the level but the stat increases can be different:

public abstract class Leveller {
    public void levelUp(Player player) {
        this.incrementStats(player);

        player.setLevel(player.getLevel() + 1); //<-- common levelup code
    }

    protected abstract void incrementStats(Player player); //<-- to be implemented by subclasses
}

Now we can split the stat progression

public class MagicLeveller extends Leveller {
    public void incrementStats(Player player) {
        player.getStats().setHp(player.getStats().getHP() + player.getLevel() + 5);
        player.getStats().setMp(player.getStats().getMp() + player.getLevel() + 2);
        player.getStats().setStrength(player.getStats().getStrength() + player.getLevel() + 2);
        player.getStats().setAgility(player.getStats().getAgility() + player.getLevel() + 2);
        player.getStats().setResilience(player.getStats().getResilience() + player.getLevel() + 1);
        player.getStats().setDeftness(player.getStats().getDeftness() + player.getLevel() + 2);
        player.getStats().setCharm(player.getStats().getCharm() + player.getLevel() + 1);
        player.getStats().setMagicalMending(player.getStats().getMagicalMending() + player.getLevel() * 2);
        player.getStats().setMagicalMight(player.getStats().getMagicalMight() + player.getLevel() * 2);
    }
}

and

public class MightLeveller extends Leveller {
    protected void incrementStats(Player player) {
        player.getStats().setHp(player.getStats().getHP() + player.getLevel() + 10);
        player.getStats().setMp(player.getStats().getMp() + player.getLevel() + 1);
        player.getStats().setStrength(player.getStats().getStrength() + player.getLevel() + 4);
        player.getStats().setAgility(player.getStats().getAgility() + player.getLevel() + 4);
        player.getStats().setResilience(player.getStats().getResilience() + player.getLevel() + 2);
        player.getStats().setDeftness(player.getStats().getDeftness() + player.getLevel() + 1);
        player.getStats().setCharm(player.getStats().getCharm() + player.getLevel() + 1);
        player.getStats().setMagicalMending(player.getStats().getMagicalMending() + player.getLevel() * 1);
        player.getStats().setMagicalMight(player.getStats().getMagicalMight() + player.getLevel() * 1);
    }
}

The values are for illustration purposes. I'm not sure if they make sense in your case but it's just to show you now have two different progression paths. So, now we can bind these paths to the classes explicitly:

public enum PlayerClass {

    WARRIOR(
            new Stats(26, 4, 18, 4, 18, 5, 4, 0, 0),
            new MightLeveller() //<-- this class progresses like a physical fighter
    ),
    PRIEST(
            new Stats(19, 14, 9, 14, 9, 9, 7, 18, 0),
            new MagicLeveller() //<-- this class progresses like a spellcaster
    ),
    MAGE(
            new Stats(18, 16, 4,18, 7, 14, 7, 0, 18),
            new MagicLeveller() //<-- this class also progresses like a spellcaster
    )
    //...
    ; 

    private final Stats stats;
    private final Leveller leveller;

    PlayerClass(Stats stats, Leveller leveller) {
        this.stats = stats;
        this.leveller = leveller;
    }

    public Stats getStats() {
        return stats;
    }

    public Leveller getLeveller() {
        return leveller;
    }
}

In the Player class, the levelUp method can now look like this

public void levelUp() {
    this.playerClass.getLeveller().levelUp(this);
}

Which means that the Player still doesn't need to have the logic for levelling nor does it even know or care what class it is. All that logic is separated away. We can easily have more levelling mechanics, even one for for each class if we wanted. If we made added the following method to PlayerClass

public void levelUp(Player player) {
    this.leveller.levelUp(player)
}

then the Player class doesn't even need to know or care about what a Leveller is, as it would just call this.playerClass.levelUp(this). Perhaps we choose to change this in the future and we don't need to touch the Player class.

---

Just a note on delegation here: This is a good example of delegation versus inheritance. Inheritance defines a is-a relationship, while delegation defines a has-a relationship. In this case we define that:

• The concrete level up mechanic (MightLeveller) is a type of level up mechanic (extends Leveller)
• a Warrior has starting stats of 26, 4, 18, 4, 18, 5, 4, 0, 0
• it also has a levelling progression for might heroes
• the Warrior is a PlayerClass. Although that is a little less clear here as it's an enum (not the normal inheritance route)

It's worth emphasising because the delegation vs inheritance comes up a lot in object oriented programming but I find it's not explained well in the beginning, which leads to some misunderstandings later on. By structuring our classes using delegation, we have a lot of freedom about their components.

If you've noticed, this means that our player has a PlayerClass as opposed to is a PlayerClass (that would have been the case if it extended PlayerClass somehow). In this case the natural language root of is/has can be murky - it makes a bit more sense to say that "He is a Warrior" as opposed to "He has the Warrior class", although they can both be correct. And indeed, we could model both of these in the system, if we wanted. However, having the class and its associated mechanics as a delegate, allows more freedom in the system because it follows the single responsibility principle - the Player doesn't need to know about how classes exactly operate in order to function. Player only needs to know it can get its starting stats and increase its stats each level and somebody else is responsible for how that is implemented.

---

Another note about Leveller and its associated subclasses. I've chosen here to make it an abstract class. You can also opt to make it an interface and so MagicLeveller and MightLeveller would not be subclasses but implementations of the interface. Both are viable options. My decision for an abstract class here was partly to showcase the template method design pattern and thus have common functionality. For example, you can have a baseline stat growth for all classes - let's say everybody gets +1 to all stats but Might classes get further bonus to Strength and HP, while Magic classes get bonus to Magical Might. And so on.

Answer 3

I'm not a Java developer, but a quick tip I can provide is to avoid comments that don't explain anything. See:

/*
* Player Class
*/
public class Player {

What's the benefit of this comment? It says exactly the same thing as the following line of code. Also, if at some point you decide that this Player class would better fit under a different name, like Character or Entity, chances are you will forget to change the comment in which case it will straight up lie about the code.

On the subject of lying, this comment already does:

/*
* Bag object instantiation
*/
private Bag b;

This is not an instantiation of the bag object - it's only a declaration.

I understand that those comments may be helpful from a beginner perspective, before things like the class declaration, constructors, fields etc become idiomatic. Over time you'll find that if you remove comments like these you'll reduce the noise in your code and make it actually easier to follow. Might as well start early.

Answer 4

I think a key point that is still missing is the idea of separating logic from data. The stat values should be stored in some sort of text file so that they can be changed separately from compiling the game. (Maybe a CSV?)

Consider a point where you have a hundred different enemies. You don't want to have all 1400 attributes stored in code (14 attributes per enemy).

Expanding on this, you could also offload the "configuration" of each enemy to this same file. (What they are strong/weak against, what they drop, the gold/XP they are worth, their rarity,...)

This is an excellent resource that expounds on this idea a bit further: http://gameprogrammingpatterns.com/bytecode.html

Answer 5

In addition to other answers, I would like to focus on "Bag management"

1. I would extract elementary bag operations like simple adding/removing/searching an item from high level methods like: buyItem, sellItem, addItem and useItem. Following OOD principles, I think that the best place for those elementary operations is Bag class itself.

2. After this simple refactoring, it would be easier to fix some bugs/corner cases:

   • Consider buying a new item when player does not have any empty slot for it.
   • Consider an exploit by passing an item that player does not have in his inventory to useItem/sellItem methods

3. I would suggest writing some test cases for it (as you have test suite ready for use)