Java beginner exercise : Write a class "Air Plane"

Question

I had to do this exercise for a further education in which I'm currently enrolled in:

Write a Java class "Air Plane". Object-property names and types are given and compulsory. Write the according constructor and getter-, setter-method. Check within the constructor the given values for being valid.

Moreover are the following methods to implement:

• info

• load

• fillUp

• fly

• getTotalWeight

• getMaxReach

Further requirements concerning the implementation of the methods I have written into my code as comments.

Here's my Plane-class

package plane;

public class Plane 
{
    private double maxWeight;
    private double emptyWeight;
    private double loadWeight;
    private double travelSpeed;
    private double flyHours;
    private double consumption;
    private double maxFuel;
    private double kerosinStorage;

    public Plane( double maxWeight, double emptyWeight, double loadWeight,
                  double travelSpeed, double flyHours, double consumption,
                  double maxFuel, double kerosinStorage )
    {
        this.maxWeight      = maxWeight;
        this.emptyWeight    = emptyWeight;
        this.loadWeight     = loadWeight;
        this.travelSpeed    = travelSpeed;
        this.flyHours       = flyHours;
        this.consumption    = consumption;
        this.maxFuel        = maxFuel;
        this.kerosinStorage = kerosinStorage < this.maxFuel
                                ? kerosinStorage
                                : this.maxFuel;
    }

    public double getMaxWeight()
    {
        return maxWeight;
    }

    public double getEmptyWeight()
    {
        return emptyWeight;
    }

    public double getLoadWeight()
    {
        return loadWeight;
    }

    public double getTravelSpeed()
    {
        return travelSpeed;
    }

    public double getFlyHours()
    {
        return flyHours;
    }

    public double getConsumption()
    {
        return consumption;
    }

    public double getMaxFuel()
    {
        return maxFuel;
    }

    public double getKerosinStorage()
    {
        return kerosinStorage;
    }

    public void setMaxWeight(double maxWeight)
    {
        this.maxWeight = maxWeight;
    }

    public void setEmptyWeight(double emptyWeight)
    {
        this.emptyWeight = emptyWeight;
    }

    public void setLoadWeight(double loadWeight)
    {
        this.loadWeight = loadWeight;
    }

    public void setTravelSpeed(double travelSpeed)
    {
        this.travelSpeed = travelSpeed;
    }

    public void setFlyHours(double flyHours)
    {
        this.flyHours = flyHours;
    }

    public void setConsumption(double consumption)
    {
        this.consumption = consumption;
    }

    public void setMaxFuel(double maxFuel)
    {
        this.maxFuel = maxFuel;
    }

    public void setKerosinStorage(double kerosinStorage) 
    {
        this.kerosinStorage = this.kerosinStorage + kerosinStorage > maxFuel
                ? maxFuel : this.kerosinStorage + kerosinStorage;
    }

    /*
        Returns the total weight of the plane. Which is: emptyWeight + 
            weight of load + weight of kerosin. 
            Expect 1 liter Kerosin as 0.8 kg.        
    */
    public double getTotalWeight () 
    {
        return emptyWeight + loadWeight
                + (kerosinStorage * 0.8);
    }

    /*
        How far can the plane fly with the current kerosin storage?        
    */

    public double getMaxReach () 
    {        
        return (kerosinStorage / consumption) * travelSpeed;
    }

    /*
        Prevent flying further then possible (with the current kerosin) !
    */
    public boolean fly (double km) 
    {
        if (km <= 0 || getMaxReach() < km || getTotalWeight() > maxWeight)
        {
            return false;
        } 

        flyHours += (km / travelSpeed);
        kerosinStorage -= (km / travelSpeed) * consumption;

        return true;
    }

    /*
        ! The parameter 'liter' can be a negative number.
        Doesn't have to be overfilled.
        Prevent a negative number as value of the 'kerosinStorage' property !
    */
    public void fillUp (double liter) 
    { 
        if ((kerosinStorage + liter) > maxFuel)
        {
            kerosinStorage = maxFuel;
        }
        else if ((kerosinStorage + liter) < 0)
        {
            kerosinStorage = 0;
        }
        else
        {
            kerosinStorage += liter;
        }
    }

    /*
        Prevent illogical value-assignments !
    */
    public boolean load (double kg) 
    {

        if ((loadWeight + emptyWeight + kg) > maxWeight)
        {
            return false;
        }
        else if ((emptyWeight + kg) < 0)
        {
            loadWeight = 0;
            return true;
        }
        else
        {
            loadWeight += kg;
            return true;
        }
    }

    // Display flying hours, kerosin storage & total weight on t. terminal.
    public void info () 
    {
        System.out.println("Flying hours: " + flyHours + ", Kerosin: "
                + kerosinStorage + ", Weight: " + getTotalWeight());
    }
}

And my Plane-test class:

package plane;

public class TestPlane
{
    public static void main (String[] args) {
        Plane jet = new Plane( 70000, 35000, 10000,
                               800, 500, 2500, 25000, 8000);

        jet.info();

        jet.setKerosinStorage(1000);       
        System.out.println(jet.getKerosinStorage());
        System.out.println(jet.getTotalWeight());
        System.out.println("Maximal reach: " + jet.getMaxReach());

        System.out.println("Fly hours 1: " + jet.getFlyHours());       
        jet.fly(5000);       
        System.out.println("Fly hours 1: " + jet.getFlyHours());    

        jet.load(10000);        
        jet.info();       
    }
}

They let automated tests run upon the code. It passed that test but I'm still not sure about it.

So therefore: I would appreciate your comments and hints concerning my implementation of the described task.

Answer 1

Builder pattern

Consider the "builder pattern". As I started to pass in arguments to the constructor, it was hard to keep the semantics right.

The builder pattern helps the developer to

• abstract from argument input order
• handle a lot of constructor arguments
• abstract from default values that make sense
• make arguments optional and therefore avoid telescope constructors

The builder pattern has only one assertion: It doesn't matter how many arguments you passed in, it will always build a consistent object.

Avoid multiple return-statements

Return-statements are structural identical to goto-statements although they are a formalized version. What all goto-alike-statements (return, continue, break) hav in common: They are not refactoring-stable. They hinder you to apply reforings like "extract method". If you have to insert a new case in an algorithm that uses break, continue and return-statements you may have to overthink the whole algorithm so that your change will not break it.

Avoid inexpressive return values

You may see return values like true/false to indicate something has been processed well or not. These return values may be sufficient for trivial cases in trivial environments where less exceptional cases occur.

In complex environment a method execution may fail due to several reasons. A connection to the server was lost, an inconsistency on the database-side was recognized, the execution failed because of security reasons... to name only the tip of the iceberg. There modern languages introduce a concept for "exceptional" cases: Exceptions.

E.g. you have following signature:

public boolean load (double kg)

Beside you have mixed two concerns in one method (load/unload) that you treat differently (overload will not be allowed, unload will be corrected) you also try to publish success information via return value.

I suggest to not publish true or false. I suggest to have either no return value or the new value of the loadWeight. Exceptional cases I would handle with the concepts of exceptions. I would expect a signature like this:

public double load (double kg) throws OverloadedException

The OverloadedException may not be signature relevant (RuntimeException) but it expresses the intention of the method.

Beside that I would split responsibilities and introduce a method:

public double unload (double kg)

Avoid comments

If you want to make comments it is an indicator for that your code itself may not be clear enough. I intentionally said "avoid comments" but not "do not comment anything". First think about the things that will be compiled and run to be as clear as possible. Then if you think it's neccessary to comment then comment. Comments have to be maintained separately. They are "uncompiled" code and cannot be be put under test. So they may lie if they diverge from your code semantics.

E.g. you have following signature:

public void fillUp (double liter)

In your comment you mentioned that "liter" may be negative. This is an allowed value but your method signature says "fillUp". So one of them is lying. You now have two possibilities:

1. Think about a name, that abstracts from draining or filling up fuel (adjust?) so it is clear that you may have a negative argument or ...
2. ... separate the concerns (draining, filling up) into separate methods to match SRP.

The best "comment" for a "procedure", "function", "method" is a set of tests that show the usage of it so other developers can see, how your code will work in different situations. Instead of testing your object in a main-scope I suggest to make ...

Unit Tests

Following the suggestions you can do expressive unit tests:

public class TestPlane {

    /**
     * A plane's fuel can be filled up.
     */
    @Test
    public void fillUpNormal() {

        Plane plane = new PlaneBuilder().setMaxFuel(2000).setInitialKerosinStorage(1700).build();

        Assert.assertEquals(1800, plane.fillUp(100));

    }

    /**
     * A plane cannot be filled up beyond max fuel.
     */
    @Test
    public void fillUpOverfilled() {

        Plane plane = new PlaneBuilder().setMaxFuel(2000).setInitialKerosinStorage(1700).build();

        try {
            plane.fillUp(400);
            Assert.fail();
        } catch (OverfilledException e) {
            Assert.assertEquals(100, e.getOverfilledBy());
        }


    }

}

You should decide which coverage you want to aim. I prefer condition coverage over statement coverage because it forces you to keep your methods small. Methods under condtion coverage have at least 2^condition_elements of test cases. If you have long methods with several conditions your test case count may explode.

As you see in the test cases, I have comments. They describe the business rules you want to enforce.

Answer 2

validity checks

As you say, the problem statement includes this:

Check within the constructor the given values for being valid.

Here's a plane that your code does not reject as invalid:

new Plane(-5, 12, 1000, 800, 500, 2500, 25000, 8000);

Not only can maxWeight (and the other parameters) invalidly be negative, it can also be exceeded by the the loadWeight.

units

Planes travel large distances, possibly reaching countries far far away, where people might use the imperial system (or even other, more arcane ones). You have an implicit contract of what units the values should be given in. That contract is not clearly communicated. Are the weights in metric tonnes? The fuel in gallons?

fun fact: The Concorde was constructed by British and French companies, one using imperial system and the other metric. Imperial units were used for the sections designed by the British team and metric units for those designed by the French team.

And even within the metric system for example, do you use kg, t, g, … for weight? You have to clearly document what units you are expecting.

Ideally, you'd use some built-in mechanism to work with units. That would even solve the imperial/metric conversion problem. Basically speaking, the dimension of a variable is specified and then the conversion of units for that dimension. If the dimension is temperature, you can shove any value into it that's a temperature, be it °C, °F, °Ré, °Rø or whathaveyou. Sadly, the proposal linked to above was withdrawn, because nobody cared to do it. I guess there are other libraries in place that do this. It's out of scope for a beginner program and nobody expects you to use a system like this. This paragraph is just FYI.

use min()/max() for clamping

If you want to keep a value within some limits, you can do this:

this.kerosinStorage = kerosinStorage < this.maxFuel
                        ? kerosinStorage
                        : this.maxFuel;

but you can also do it this way

this.kerosinStorage = Math.min(kerosinStorage, this.maxFuel);

which is a bit more self documenting if you got used to it, because it clearly says: take the smaller value. Parsing the ternary operator is a bit harder to read.

This might be personal preference. There's also a (blurry) line where simplifying code becomes obfuscation to cryptic one-liners. Don't be tempted to write everything in as little code as possible. You have to draw that line for yourself.

the validity checks strike back

Even if you have some validity checks, you have to come up with a contract on how they are enforced. What should happen if some invalid value is passed to your class? Maybe you want to throw newIllegalArgumentException("Negative weights aren't possible.") or maybe you want to limit the values to a certain range of valid values and move on. There is no clear right or wrong here. It's a matter of how you design your class. What's important is that you communicate whatever you do clearly. Even if you don't perform any validity checks, you should explain this to possible users of your class: "Hey there, if you pass nonsense to this class, it won't check for that and you have to live whatever garbage it produces as a result." This could be a valid choice in some situations. (In yours it's obviously not, because you were asked to do them)

naming

This is a subjective one. When I read kerosinStorage, I associate with that the size of the tank or container that the kerosene is stored in. In your code it is the actual amount of kerosene left in the plane. I suggest to change that name to just kerosene.

setters should set, not add

The setter for kerosinStorage adds to the current value and doesn't set it

public void setKerosinStorage(double kerosinStorage) 
{
    this.kerosinStorage = this.kerosinStorage + kerosinStorage > maxFuel
            ? maxFuel : this.kerosinStorage + kerosinStorage;
}

Either make this method a setter or rename it to addKerosene or something like that.

the return of the validity checks

Admittedly, this is more about code duplication, but I could not resist completing the trilogy.

Check within the constructor the given values for being valid.

That's a bit misleading, because if you have setters for properties, you should perform the validity checks in the setters, not in the constructor. As you have it right now, you have duplicated code to limit the value of kerosinStorage. You do it once in the constructor:

    this.kerosinStorage = kerosinStorage < this.maxFuel
                            ? kerosinStorage
                            : this.maxFuel;

and then you do it again in "addKerosene":

    this.kerosinStorage = this.kerosinStorage + kerosinStorage > maxFuel
            ? maxFuel : this.kerosinStorage + kerosinStorage;

Here's a reduced version of your class, that does things differently:

package plane;

public class Plane 
{
    private double maxFuel;
    private double kerosene;

    public Plane(double maxFuel, kerosene)
    {
        this.maxFuel        = maxFuel;
        setKerosene(kerosene);
    }

    public void setKerosene(double amount)
    {
        kerosene = Math.min(maxFuel, amount);
    }

    public void addKerosene(double amount) 
    {
        setKerosene(kerosene + amount);
    }
}

In your current code, you have some validity checks in the setter methods fillUp() and load(), but they are not called in the constructor.

To wrap up the whole validity check thing once and for all:

• perform validity checks in the setter methods
• call the setter methods from the constructor to ensure they are performed and to prevent code duplication
• come up with a consistent way to deal with invalid values and communicate it clearly

Answer 3

Don't make your Class members private if you offer public getters and setters without any functional logic. Just make your members public. This saves you 70 (i.e. 7*2*5) lines if I counted correctly. It makes your class more readable, the access of the variables less complicated and saves Bytecode.