Skip to main content
deleted 1 character in body
Source Link
Pimgd
  • 22.3k
  • 5
  • 66
  • 144

No unit defined, so tracking down business logicalogic bugs is hard. Put it in comments, or, even better, in the function name. Something like getFuelCapacityInLiters().

No unit defined, so tracking down business logica bugs is hard. Put it in comments, or, even better, in the function name. Something like getFuelCapacityInLiters().

No unit defined, so tracking down business logic bugs is hard. Put it in comments, or, even better, in the function name. Something like getFuelCapacityInLiters().

added 3498 characters in body
Source Link
Pimgd
  • 22.3k
  • 5
  • 66
  • 144

SmallCar...

    // returns the number of whole Litres of fuel consumed during the journey
    public int drive(int distance) {
        int fuelUsed = 0;

        if (distance < 0) {
            throw new IllegalArgumentException("Distance cannot be less than zero");
        }

        if (super.isRented() && (super.getCurrentFuel() > 0)) {
            fuelUsed = (distance / 25);
            super.setCurrentFuel(super.getCurrentFuel() - fuelUsed);
            return fuelUsed;
        }
        return fuelUsed;
    }

Multiple things that can be better here:

First, guard clauses like

        if (distance < 0) {
            throw new IllegalArgumentException("Distance cannot be less than zero");
        }

They go at the top of the function where possible. Before any variable declarations. No need to reserve memory for something that you're not going to use. Also, it allows you to first perform checks, then have ALL the code below assume sane values.

if (super.isRented() && 

Apparently, non-rented cars cannot drive. But yours don't notify the caller. They'll happily report the trip as having taken 0 liters of fuel. I want to buy one of those cars. Not having to pay for fuel would be great.

fuelUsed = (distance / 25);

Every car has a different milage, and yours is infinite kilometers per liter, provided the car has a liter of fuel to start with, and that the trip doesn't go over 25 kilometers. I've got about a 20km commute, so that's great. I REALLY want one of these cars. Joking aside, integer division is a real problem with this sort of thing and that's why distances and amounts of an item of which there can only be many (such as water or fuel, you do not have 1 fuel, you have 1 liter or 0.5 or 0.05 liter of fuel), tend to be floating point numbers. Real numbers. You'll probably have to use double for them.

            return fuelUsed;
        }
        return fuelUsed;

This is redundant.


    public boolean isFull() {
        boolean bool = super.getIsFull();
        return bool;
    }

Why do you have this method? People can just call getIsFull (or isTankFull or maybe hasFullTank, see points on naming earlier).


SmallCar...

    // returns the number of whole Litres of fuel consumed during the journey
    public int drive(int distance) {
        int fuelUsed = 0;

        if (distance < 0) {
            throw new IllegalArgumentException("Distance cannot be less than zero");
        }

        if (super.isRented() && (super.getCurrentFuel() > 0)) {
            fuelUsed = (distance / 25);
            super.setCurrentFuel(super.getCurrentFuel() - fuelUsed);
            return fuelUsed;
        }
        return fuelUsed;
    }

Multiple things that can be better here:

First, guard clauses like

        if (distance < 0) {
            throw new IllegalArgumentException("Distance cannot be less than zero");
        }

They go at the top of the function where possible. Before any variable declarations. No need to reserve memory for something that you're not going to use. Also, it allows you to first perform checks, then have ALL the code below assume sane values.

if (super.isRented() && 

Apparently, non-rented cars cannot drive. But yours don't notify the caller. They'll happily report the trip as having taken 0 liters of fuel. I want to buy one of those cars. Not having to pay for fuel would be great.

fuelUsed = (distance / 25);

Every car has a different milage, and yours is infinite kilometers per liter, provided the car has a liter of fuel to start with, and that the trip doesn't go over 25 kilometers. I've got about a 20km commute, so that's great. I REALLY want one of these cars. Joking aside, integer division is a real problem with this sort of thing and that's why distances and amounts of an item of which there can only be many (such as water or fuel, you do not have 1 fuel, you have 1 liter or 0.5 or 0.05 liter of fuel), tend to be floating point numbers. Real numbers. You'll probably have to use double for them.

            return fuelUsed;
        }
        return fuelUsed;

This is redundant.


    public boolean isFull() {
        boolean bool = super.getIsFull();
        return bool;
    }

Why do you have this method? People can just call getIsFull (or isTankFull or maybe hasFullTank, see points on naming earlier).

added 3498 characters in body
Source Link
Pimgd
  • 22.3k
  • 5
  • 66
  • 144

In AbstractVehicle...

public void isTankFull() {
    if ((currentFuel - fuelCapacity) >= 0) {
        isFull = true;
    } else
        isFull = false;
}

// Calls the isTankFull method and then returns isFull
public boolean getIsFull() {
    isTankFull();
    return isFull;
}

What is all this code?

if ((currentFuel - fuelCapacity) >= 0) {
    isFull = true;
} else
    isFull = false;

This is missing braces which you should add because they are a sanity check - missing braces means unfinished thoughts means unfinished logic - at least for me, and I freak out whenever the braces are missing.

if ((currentFuel - fuelCapacity) >= 0) {
    isFull = true;
} else {
    isFull = false;
}

But it is redundant... because this might as well be

if (theThing == true) {
    isFull = true;
} else {
    isFull = false;
}

So why not...

isFull = (currentFuel - fuelCapacity) >= 0;

Except that, looking at the conditional, there is no need to compare to 0, you can just compare to other values.

isFull = currentFuel >= fuelCapacity;

...

public void isTankFull() {
    isFull = currentFuel >= fuelCapacity;
}

// Calls the isTankFull method and then returns isFull
public boolean getIsFull() {
    isTankFull();
    return isFull;
}

This is still silly, of course, there is no need to keep track of internal state of isFull, it is trivially derivable:

public boolean getIsFull() {
    return currentFuel >= fuelCapacity;
}

There!

Except the naming is flawed; this should really be called isTankFull. Adjust your interface as needed here. getIsX is a really weird construction.


In AbstractVehicle...

public void isTankFull() {
    if ((currentFuel - fuelCapacity) >= 0) {
        isFull = true;
    } else
        isFull = false;
}

// Calls the isTankFull method and then returns isFull
public boolean getIsFull() {
    isTankFull();
    return isFull;
}

What is all this code?

if ((currentFuel - fuelCapacity) >= 0) {
    isFull = true;
} else
    isFull = false;

This is missing braces which you should add because they are a sanity check - missing braces means unfinished thoughts means unfinished logic - at least for me, and I freak out whenever the braces are missing.

if ((currentFuel - fuelCapacity) >= 0) {
    isFull = true;
} else {
    isFull = false;
}

But it is redundant... because this might as well be

if (theThing == true) {
    isFull = true;
} else {
    isFull = false;
}

So why not...

isFull = (currentFuel - fuelCapacity) >= 0;

Except that, looking at the conditional, there is no need to compare to 0, you can just compare to other values.

isFull = currentFuel >= fuelCapacity;

...

public void isTankFull() {
    isFull = currentFuel >= fuelCapacity;
}

// Calls the isTankFull method and then returns isFull
public boolean getIsFull() {
    isTankFull();
    return isFull;
}

This is still silly, of course, there is no need to keep track of internal state of isFull, it is trivially derivable:

public boolean getIsFull() {
    return currentFuel >= fuelCapacity;
}

There!

Except the naming is flawed; this should really be called isTankFull. Adjust your interface as needed here. getIsX is a really weird construction.

added 3498 characters in body
Source Link
Pimgd
  • 22.3k
  • 5
  • 66
  • 144
Loading
added 630 characters in body
Source Link
Pimgd
  • 22.3k
  • 5
  • 66
  • 144
Loading
Source Link
Pimgd
  • 22.3k
  • 5
  • 66
  • 144
Loading