-1
\$\begingroup\$

Let's say I have an abstract class Character that have fields health and maxHealth. maxHealth differs between subclasses. However, the first way I implemented it the health is always initialized as equals to maxHealth.

public abstract class Character {
  protected Vector position;
  protected int health;
  protected int maxHealth;

  public Character(float xLocation, float yLocation){
      Vector position = new Vector(xLocation, yLocation);
      health = maxHealth;
  }
}

public class Grunt extends Character {
  public Grunt(float xLocation, float yLocation){
      super(xLocation, yLocation);
      maxHealth = 200;        
  }
}

But this didn't work correctly because health is assigned to maxHealth before maxHealth was initialized as 200.

I tried to change constructor parameters as follows and it works but I'm not sure that if this is a good practice. What do you think?

public abstract class Character {
    protected Vector position;
    protected int health;
    protected int maxHealth;

    public Character(float xLocation, float yLocation, int maxHealth){
        Vector position = new Vector(xLocation, yLocation);
        this.maxHealth = maxHealth;
        health = maxHealth;
    }
}

public class Grunt extends Character {
    public Grunt(float xLocation, float yLocation){
        super(xLocation, yLocation, 200);
    }
}
\$\endgroup\$
  • \$\begingroup\$ What is this: new Vector(xLocation, yLocation)? There is no constructor Vector(float, float), or Vector(double, double) (which would also accept two floats). \$\endgroup\$ – Stingy May 2 '18 at 9:38
  • \$\begingroup\$ @Stingy, there's not even a type Vector in scope at that point, so we can't even guess what constructors it might have. \$\endgroup\$ – Toby Speight May 2 '18 at 12:07
  • \$\begingroup\$ @TobySpeight I assumed Vector refers to java.util.Vector and the import was not included in the code sample. But you're right, theoretically, Vector might also refer to an entirely different class. \$\endgroup\$ – Stingy May 2 '18 at 12:42
  • \$\begingroup\$ @Stingy: and I wasn't reading properly when I came here from Review (too used to reading C++ code). It might be a typo for javax.vecmath.Vector2f, of course (but without any imports, we're still guessing). \$\endgroup\$ – Toby Speight May 2 '18 at 12:58
  • \$\begingroup\$ Vector class is written by me and it represents the geometrical 2D vectors with x and y coordinates. \$\endgroup\$ – Emre Sülün May 9 '18 at 7:25
0
\$\begingroup\$

Another way, to be able to change the value later is to use a setter. MaxHealth could change in case of any bonus you might implements.

public void setMaxHealt(int maxHealth){
    this.maxHealth = maxHealth;
    this.health = maxHealth;
}

And in the constructor, simply use the setter.

public Grunt(float xLocation, float yLocation){
     super(xLocation, yLocation);
     setMaxHealth(200);
}

Using the constructor is fine, and is a good solution to be honest but when you will find constructor with 20 parameters... you will need other solution.

Note :
Since I said this could be nice to add this setter for a bonus, if you don't want to "heal" your character if is health wasn't full, you can simply check before change the value if this.health == this.maxHealth and set the value based on that.

\$\endgroup\$
2
\$\begingroup\$

Instead of questioning whether it is a good practice, it would be more helpful to be aware of what your first code actually does. JLS §15.9.4 has this to say about the creation of an object:

The new object contains new instances of all the fields declared in the specified class type and all its superclasses. As each new field instance is created, it is initialized to its default value (§4.12.5).

This means that health and maxHealth are initialized to 0 even before the constructor is executed. So your statement health = maxHealth; is completely pointless, because health is only assigned the default value which is already assigned to it.

I see no problem with your second version (apart from the usage of a non-existent Vector constructor, which I already hinted at in a comment). Every field is initialized in the constructor of the class in which it is declared, which, although not strictly necessary for non-final fields, still makes the code easier to read because you don't have to jump around classes to understand the fields' initializations.

Also, the class Vector is antiquated. If you don't need synchronization, you'd be better off with an ArrayList (and if you do, there are still better solutions as explained in the link). Besides, you are using a raw type, thereby denying the additional type-safety provided by generics.

Edit

Actually, there is a problem in both of your versions that I hadn't spotted earlier: Your constructors never assign a value to the instance variable position. Instead, they declare a local variable with the name position that hides the instance variable. To rectify this, you need to remove the type declaration Vector from the assignment, so that this:

Vector position = new Vector(xLocation, yLocation); //declares a local variable unrelated to `Character.position`

Becomes this:

position = new Vector(xLocation, yLocation); //refers to Character.position
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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