15
\$\begingroup\$

I have written this code representing a class hierarchy of character classes, and a world class to print their properties.

Is anything unclear about this code? Am I utilizing good practices?

class Human{    
    protected String name;
    protected int health;
    protected int armorLevel;
    protected int magicLevel;
    protected int experience;
    protected int level;
        Human(){
        name = "Andrew The Magic";
        health = 100;
        armorLevel = 1;
        magicLevel = 1;
        experience=0;
        level =1;
        }

    Human(String name){
        this(); //calling main constructor
        this.name = name;
    }

void fight() {}
void levelUp() {}
void showAbility() { System.out.println("Name: "+name+" HP: "+health+" ARMOR: "+armorLevel+" MAGIC: "+magicLevel+" EXP: "+experience+" LVL: "+level); }
void yell(){    System.out.println("IM A CHARACTER ;-(");   }
}

//--------------------------------------------------------------        
class Warrior extends Human{

    Warrior(){ 
        super();    
    }

    Warrior(String name){
        super(name);
    }

    void fight(){
        System.out.println("I attack with sword");
        experience +=10;
        if (experience >= 20 ){ levelUp();  }
        }

    void levelUp(){
        System.out.println("LEVEL UP!");
        health +=100;
        armorLevel +=25;
        level++;
    }

    void yell(){
        System.out.println("IM A GLORIOUS WARRIOR "+name);
    }
}    
//--------------------------------------------------------------

public class World{


    static void greeting(Human p){
        p.yell();
    }

        public static void main(String[] args){
          Warrior w = new Warrior("Noname the Warrior");
            w.showAbility();
            w.fight();
            w.showAbility();
            w.fight();
            w.showAbility();

            greeting(w);
   }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Please add more context to your code. Describing a bit about what the code does will prevent you from getting down-votes (and currently also some close votes) \$\endgroup\$ Dec 20, 2014 at 13:33

6 Answers 6

14
\$\begingroup\$

Is anything unclear about this code?

The purpose of this code is unclear.

Do I break any good-programming rules?

Some bad practices that immediately jump into the eye:

  • Formatting is horrible
  • The parameterless constructor of Warrior is pointless and can be safely omitted
  • Methods with empty bodies, such as fight and levelUp in Human should at least have a comment explaining why the implementation is left empty
  • showAbility is poorly named:

    • None of the attributes are abilities: name is certainly not an ability, and the other fields are questionable too
    • "show" is a bit ambiguous. Show where? In a pop-up, maybe? I'd use the word "print" instead.

    Maybe printStats would be better.

  • It's probably not a big concern for your programming exercise, but for the record, print statements inside a program are generally frowned upon. The preferred way is to use a logging framework instead. (Again, this is just for the record, since you asked about bad practices.)

Instead of Warrior inheriting from Human, consider using interfaces. For example you could have Warrior and Mage interfaces, so that you can have mighty mage-warriors. With inheritance this would be impossible. Using interfaces will lead to a more extensible design.

\$\endgroup\$
8
\$\begingroup\$

It's hard to tell just from this code sample what you hope to accomplish with the Human-Warrior inheritance. Specifically, I am puzzled by the methods Human.fight() and Human.levelUp() that have empty bodies. If Humans are unable to fight, perhaps there shouldn't be a fight() method there at all? Alternatively, if your game is to have multiple classes of fighters (e.g. warriors, knights, and spies), none of which is a plain wimpy human, then the Human class and the placeholder methods should be made abstract instead.

Reviewing just the Human class, I would write it more like this:

class Human {
    protected String name;
    protected int health = 100;
    protected int armorLevel = 1;
    protected int magicLevel = 1;
    protected int experience = 0;
    protected int level = 1;

    Human() {
        this("Andrew The Magic");
    }

    Human(String name) {
        this.name = name;
    }

    void fight() {}   // Dubious, as mentioned above
    void levelUp() {} // Dubious, as mentioned above

    void showAbility() {
        System.out.printf("Name: %s HP: %d ARMOR: %d MAGIC: %d EXP: %d LVL: %d\n",
                          name, health, armorLevel, magicLevel, experience, level);
    }

    void yell() {
        System.out.println("IM A CHARACTER ;-(");
    }
}

Notable points are:

  • Set the default values at the declaration site, to avoid duplication and prevent accidental mismatches.
  • Chain the default constructor to the explicit constructor, so that the name is set correctly the first time. (You could even make name a final variable.)
  • Use System.out.printf() as a more readable alternative to string concatenation.
\$\endgroup\$
2
  • \$\begingroup\$ "It's hard to tell just from this code sample what you hope to accomplish with the Human-Warrior inheritance. Specifically, I am puzzled by the methods Human.fight() and Human.levelUp() that have empty bodies. " - In my mind, there is no true Human in game, there are only Human subclasses like Human Warrior, Human Mage etc... I wanted to create a Shape for all of the subclasses so i can for example call fight method for different subclass very similarly. For example if i got a reference w = new Warrior(); and m = new Mage(); i can call fight(m); and fight(w); similarly for all characters. \$\endgroup\$
    – Jacob
    Dec 20, 2014 at 6:18
  • 2
    \$\begingroup\$ So Human should be an abstract class then. \$\endgroup\$ Dec 20, 2014 at 6:22
8
\$\begingroup\$

The first thing you are destroying is the formatting. After formatting, it looks like this:

class Human {
    
    protected String name;
    protected int health;
    protected int armorLevel;
    protected int magicLevel;
    protected int experience;
    protected int level;

    Human() {
        name = "Andrew The Magic";
        health = 100;
        armorLevel = 1;
        magicLevel = 1;
        experience = 0;
        level = 1;
    }

    Human(String name) {
        this(); // calling main constructor
        this.name = name;
    }

    void fight() {
    }

    void levelUp() {
    }

    void showAbility() {
        System.out.println("Name: " + name + " HP: " + health + " ARMOR: "
                + armorLevel + " MAGIC: " + magicLevel + " EXP: " + experience
                + " LVL: " + level);
    }

    void yell() {
        System.out.println("IM A CHARACTER ;-(");
    }
}

// --------------------------------------------------------------
class Warrior extends Human {

    Warrior() {
        super();
    }

    Warrior(String name) {
        super(name);
    }

    void fight() {
        System.out.println("I attack with sword");
        experience += 10;
        if (experience >= 20) {
            levelUp();
        }
    }

    void levelUp() {
        System.out.println("LEVEL UP!");
        health += 100;
        armorLevel += 25;
        level++;
    }

    void yell() {
        System.out.println("IM A GLORIOUS WARRIOR " + name);
    }
}

// --------------------------------------------------------------

public class World {

    static void greeting(Human p) {
        p.yell();
    }

    public static void main(String[] args) {
        Warrior w = new Warrior("Noname the Warrior");
        w.showAbility();
        w.fight();
        w.showAbility();
        w.fight();
        w.showAbility();

        greeting(w);
    }
    
}

You do not have to create empty methods just so another class can override them. Just simply remove them:

class Human {
    
    protected String name;
    protected int health;
    protected int armorLevel;
    protected int magicLevel;
    protected int experience;
    protected int level;

    Human() {
        name = "Andrew The Magic";
        health = 100;
        armorLevel = 1;
        magicLevel = 1;
        experience = 0;
        level = 1;
    }

    Human(String name) {
        this(); // calling main constructor
        this.name = name;
    }

    void showAbility() {
        System.out.println("Name: " + name + " HP: " + health + " ARMOR: "
                + armorLevel + " MAGIC: " + magicLevel + " EXP: " + experience
                + " LVL: " + level);
    }

    void yell() {
        System.out.println("IM A CHARACTER ;-(");
    }
}

Also, I assume a normal human will not need the following stats:

protected int health;
protected int armorLevel;
protected int magicLevel;
protected int experience;
protected int level;

Just move them to the Warriors class, and move the assignments as well:

class Human {
    
    protected String name;

    Human() {
        name = "Andrew The Magic";
    }

    Human(String name) {
        this(); // calling main constructor
        this.name = name;
    }

    void showAbility() {
        System.out.println("Name: " + name);
    }

    void yell() {
        System.out.println("IM A CHARACTER ;-(");
    }
}

// --------------------------------------------------------------
class Warrior extends Human {

    protected int health;
    protected int armorLevel;
    protected int magicLevel;
    protected int experience;
    protected int level;
    
    Warrior() {
        super();
        health = 100;
        armorLevel = 1;
        magicLevel = 1;
        experience = 0;
        level = 1;
    }

    Warrior(String name) {
        super(name);
        health = 100;
        armorLevel = 1;
        magicLevel = 1;
        experience = 0;
        level = 1;
    }

    void fight() {
        System.out.println("I attack with sword");
        experience += 10;
        if (experience >= 20) {
            levelUp();
        }
    }

    void levelUp() {
        System.out.println("LEVEL UP!");
        health += 100;
        armorLevel += 25;
        level++;
    }

    void yell() {
        System.out.println("IM A GLORIOUS WARRIOR " + name);
    }
}

Obviously you don't want to repeat code, so change the constructors to:

Warrior() {
    this("Andrew The Magic");
}

Warrior(String name) {
    super(name);
    health = 100;
    armorLevel = 1;
    magicLevel = 1;
    experience = 0;
    level = 1;
}

Also, you don't have to call the constructor that takes no parameters in the Human class. If you do, the first constructor will assign "Andrew The Magic" to name, then assign the parameter passed as name to name.

Human() {
    name = "Andrew The Magic";
}

Human(String name) {
    this.name = name;
}

The method static void greeting(Human p) is unnecessary. I suggest remove the method and just call yell().

Final Code:

class Human {
    
    protected String name;

    Human() {
        name = "Andrew The Magic";
    }

    Human(String name) {
        this.name = name;
    }

    void showAbility() {
        System.out.println("Name: " + name);
    }

    void yell() {
        System.out.println("I'M A CHARACTER ;-(");
    }
}

// --------------------------------------------------------------
class Warrior extends Human {

    protected int health;
    protected int armorLevel;
    protected int magicLevel;
    protected int experience;
    protected int level;
    
    Warrior() {
        this("Andrew The Magic");
    }

    Warrior(String name) {
        super(name);
        health = 100;
        armorLevel = 1;
        magicLevel = 1;
        experience = 0;
        level = 1;
    }

    void fight() {
        System.out.println("I attack with sword");
        experience += 10;
        if (experience >= 20) {
            levelUp();
        }
    }

    void levelUp() {
        System.out.println("LEVEL UP!");
        health += 100;
        armorLevel += 25;
        level++;
    }

    void yell() {
        System.out.println("IM A GLORIOUS WARRIOR " + name);
    }
}

// --------------------------------------------------------------

public class World {

    public static void main(String[] args) {
        Warrior w = new Warrior("Noname the Warrior");
        w.showAbility();
        w.fight();
        w.showAbility();
        w.fight();
        w.showAbility();

        w.yell();
    }
    
}
\$\endgroup\$
5
  • \$\begingroup\$ wow, I see that I need to read some good codes. You made from that short code a sweet poem \$\endgroup\$
    – Jacob
    Dec 20, 2014 at 5:21
  • \$\begingroup\$ "Also, I assume a normal human will not need the following stats" - but if I would have a class Mage that extends Human, wouldnt be better to have them ? \$\endgroup\$
    – Jacob
    Dec 20, 2014 at 5:25
  • \$\begingroup\$ @Jacob Then I suggest you create a class that has those stats, and then extend Warrior and Mage from that. \$\endgroup\$ Dec 20, 2014 at 18:02
  • 1
    \$\begingroup\$ @MannyMeng Isn't that what the Human class was for? Also, I checked, (in java8) if you cast a Warrior to Human and call yell(), the overriding method inside Warrior will be executed, NOT the one inside Human. \$\endgroup\$ Dec 20, 2014 at 19:37
  • \$\begingroup\$ @MannyMeng I get the impression that you think the Human class is for general characters, while the Warrior class is for characters who will show up in battle. A lot of your advice is poor if the Human class is actually the superclass for characters who appear in battle. (Although it's not clear which is the case from the OP's question.) \$\endgroup\$
    – Kevin
    Dec 21, 2014 at 2:55
7
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

Much has been covered already, so I'll just drop a note about these comments:

// --------------------------------------------------------------

These are just clutter, remove them mercilessly.

I come with a C# perspective though; I find it cleaner when there a class per file. It gets easier to navigate your project, too - I would think the same applies to Java.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ It definitely applies the same to Java \$\endgroup\$ Dec 20, 2014 at 13:41
4
\$\begingroup\$

Going by the topic "Printing object properties" : - Override toString() in your POJOs and use the same wherever required i.e

public String toString(){
   return "Name: " + this.name; // Going by Manny Meng's feedback
}
void showAbility(){
   System.out.println(this);
}

//Warrior class.
public String toString(){
   return super.toString() + "HP: " + this.health; //Add the rest.
}
  • Try using the member variable as private. Link
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.