I also enjoy writing RPG games, and so will provide some input based on my experience. There are several other good answers here that I read who cover things I will say, but I'll try not to overlap.
First, you say you have a class hierarchy. That's only half-true. You have a package-access class Human
. When I hear someone talking bout hierarchies in an OO design, I expect to see interfaces or abstract classes. I also wonder, would a Dwarf
or Elf
object not have the same properties (i.e. health
, armorLevel
etc.) as a Human
object? There should be an interface at a higher level of abstraction that defines this.
Additionally, I'm adverse to protected instance variables. These should be private, and the interface should provide accessors. Consider the following:
public interface Entity {
int getHealth();
int getArmourLevel();
..
int getLevel();
}
And then you can you a skeletal implementation of the interface:
public abstract AbstractEntity {
private int health;
private armourLevel;
...
private level;
@Override
public getHealth() {
return health;
}
..
}
I here used the name Entity
as whatever you call whatever the generic term of Humans, Elves, or Dwarves would be. You could use whatever other word you want. Also Google Effective Java Item 18 for why you may want to do this.
Moving down, I see your default constructor Human()
. There's no reason for this in my opinion. From your current implementation, you should combine your two, and only take the object's name:
public Human(String name) {
this.name = name;
this.health = 100;
this.armorLevel = 1;
this.magicLevel = 1;
this.experience = 0;
this.level = 1;
}
A better implementation would have constants which defined the default value of these fields, such as private static final int DEFAULT_HEALTH = 100;
so you can modify these later without much concern of changing these values every where.
The way you handle your second constructor, Human(String name)
, is also non-conventional in Java. You are going from a more specific constructor to a less specific one. It is conventionally the other way. For example, your constructor Human(String name)
then calls Human(String name, int health, ...)
.
You are in a nice position to employ the Builder pattern to construct your Human
objects, but this might be overkill if you expect most of your objects to be created with the default values.
Moving to the end of this class, I see four methods. There's two things wrong here: first,the Human
class is a data class and so should only have methods which modify it's state. In other words: getters and optionally setters. Another module should be in charge of the fight and level up logic, etc.
Also, fight
and levelUp
don't do anything, so make them abstract
. If you follow the skeletal implementation suggestion above, than all of these methods can be moved there if necessary.
Moving on, a Warrior
isn't necessarily a Human
, is it? I may just be assuming that there are more races than humans because you specify Human specifically.
Looking further into the implementations of the Warrior
class's methods I can see some more things.
System.out.println("I attack with sword");
This might be picky, but this is output or view information, and doesn't have to do with the fight method. Here's an alternative:
public interface Entity {
...
void fight();
String getFightMessage();
...
}
public abstract class AbstractEntity implements Entity {
@Overrides
public fight() {
System.out.println(getFightMessage());
....
}
}
public class Warrior extends AbstractEntity {
@Overrides getFightMessage() {
return "I attack with sword";
}
}
I suggest the you next come up with some Monster
class which your hero can fight. You can then pass as an argument into the fight
method. And make sure you come back with your code when you do so! But that being said, your Human
class shouldn't be concerned with how combat is handled -- you should really have a separate class which handles all of that logic. Think of your Human
objects as objects which have thing done to them, rather than things which affect other objects.