Generating RPG Characters (Objects)

I am a beginner Java programmer. I have just finished up an assignment and would appreciate some advice and/or constructive criticism on my program. I am trying to ensure I do not advance my knowledge using bad practices. For context, I have the assignment details, followed by the full code below:


Part 1: Write an abstract Human class that implements the Human interface above. You’ll need to include the interface in your project.


Part 2: Implement the Knight and Wizard classes.


Part 3: Write a test program called LastNameWorld, which:

• Declares 4 Humans

• Instantiates a Knight

• Instantiates a Wizard and moves it

• Clones the Knight and changes its name

• Checks if the Knight clone is not equal to the original Knight, and if so: moves the clone

• Clones the Wizard

• Checks if the Wizard clone is equal to the original Wizard, and if so: moves the clone

• Prints the total number of Humans living in this World

• Prints a description (toString) of all Humans to the console

Part 4: Add a third Human (e.g. Archer, King, Queen, Paladin, etc) type class to your simulation. This class must contain an equals and a clone method. Add code to your test harness to fully test your new class.

Main Class

```
public class BakosWorld {

public static void main(String[] args) {

// Declare 4 humans
Human[] humans = new Human[4];

// Create original knight and wizard, then move wizard
humans[0] = new Knight("Rob", 36, 100, 6.1, 50, 50, "Grey Wind");

humans[1] = new Wizard("Gandalf", 32, 80, 5.11, 25, 25, 50);
humans[1].move(45, 45);

// Creating a new knight, that is a clone of the first knight
humans[2] = ((Knight) humans[0]).clone();
// Checks to see if the two knights are NOT equal
if (((Knight) humans[0]).equals(((Knight) humans[2])) == false) {
humans[2].move(85, 85);
}

// Creating a new wizard, that is a clon

Your code is quite well structured. A few points:

Useless cast before calling a method

These cast bother me:

// Creating a new knight, that is a clone of the first knight
humans[2] = ((Knight) humans[0]).clone();
// Checks to see if the two knights ARE equal
if (((Wizard) humans[1]).equals(((Wizard) humans[3]))) { //...

The clone method will clone the object. Whatever their type, they'll be cloned to the same class, be it Wizard, Assassin, or whatever. But you don't need to know their exact type to compare them using equals, because that method conveniently only requires the compared object to be Object.

So this does the same thing:

// Creating a new knight, that is a clone of the first knight
humans[2] = humans[0].clone();
// Checks to see if the two knights ARE equal
if (humans[1]).equals(humans[3])) { //...

But for that to work, you'd need to have a proper equals and a proper clone methods...

Clonable interface

From the Java API documentation on clone():


[...] if the class of this object does not implement the interface Cloneable, then a CloneNotSupportedException is thrown.

So you need to decide who should carry out this function (the Human interface, the Abstract class or whatnot) and make it extend/implement cloneable.

Implement equals properly

Your declaration of equals is unorthodox, and probably does not do what you're thinking You are probably expected to Override the Object.equals(Object obj) method. Right now, your Human objects cannot compare theselves. Only Assassin with Assassin, Wizard with Wizard, etc. but not Assassin versus Wizard.

So when you call assassin.equals(otherAssassin), you're calling your own function Assassin.equals. But when calling human.equals(otherHuman) you're calling the Object.equals(Object), and it might not suit you perfectly because your did not override it. This is the reason you needed to cast earlier... now you won't!

I propose that you shed a lot of boilerplate by centralizing the tedious parts of the usual equals implementation (type checking, null checking etc.), by using double dispatch :

public abstract class Human implements HumanInterface {
    [...blah blah blah]
    public boolean equals(Object 
        if (other != null && other instanceof Human) {
            Human otherHuman = (Point) obj;
            return otherHuman.equals(this); // Order is important!
        }else{
            return false;
        }
    }

    // I like to provide default implementation, but at least one MUST be overriden
    public boolean equals(Knight knight){ return false; }
    public boolean equals(Wizard wizard){ return false; }
    public boolean equals(Assassin assassin){ return false; }

}

And then each concrete class only has to override their corresponding method:

public class Knight extends Human {
    [...blah blah blah...]
    @Override
    public boolean equals(Knight knight){
         [...your own implmentation here...]
    }
}

You see, toString, clone, equals and hashCode are methods of Object, so absolutely ALL classes must abide by the contract defined by the Object class. And you still have that last one to code...

This also means the public String toString(); declaration in HumanInterface is useless.

String handling

message = "\n[ASSASSIN]"
                + "\nxPos: " + xPos
                + "\nyPos: " + yPos
                + "\nName: " + name
                + "\nAge: " + age
                + "\nHealth: " + health
                + "\nPoison: " + poison
                + "\nHeight: " + height;

Please use a StringBuilder when concatenating many Strings, or (better here) a String.format to make this safe and more efficient.

Comment clutter

// Assassin toString method
public String toString() {

And

// Check to see if 2 objects are the same
public boolean equals(Assassin a) {

These comment brings nothing, remove it, or turn it into a proper Javadoc. For toString, its probably useless.

But inflictPoison(Human h) would need some. Especially since it can make poison go negative... and still work.

Storing the humans

Why use an Array, and not a List? you create and kill humans, it would make sense. You would then also be able to remove() them when dead.

Then counting them would be as simple as list.size();

Moreover, I would rather keep an individual (correctly typed) reference to each of them, s that their special skills are available without casting!

This would read way better:

```
// Create original knight and wizard, then move wizard
Knight rob = new Knight("Rob", 36, 100, 6.1, 50, 50, "Grey Wind");
Wizard gandalf = new Wizard("Gandalf", 32, 80, 5.11, 25, 25, 50);
rob.move(45, 45);

// Creating a new knight, that is a clone of the first knight
Knight robClone = rob.clone();
// Checks to see if the two knights are NOT equal
if (!rob.equals(robclone)) {
robclone.move(85, 85);
}

StackExchange Code Review Q#148133, answer score: 2