this.stringRep = String.format("%s%04d", letter, numbers);
This could have been in your getter `getStringRepresentation` - there is no real need to store this in memory. What does it mean if you have a `RegistrationNumber` of "A1234" with the letter "A" and the number 2345? It'd be inconsistent. Your design doesn't allow this inconsistency to occur (through the use of a single constructor and `final`), so it's a minor point, but I don't see the need to have this variable.
---
if ((lastName == null) || (lastName.isEmpty())) {
throw new IllegalArgumentException("lastName cannot be null or empty");
}
You could make a helper function for this that takes two strings, one to check and the other to put as variable name in the exception message. Alternatively, usually in enterprise projects you'd already have a helper function for this; both Guava and Apache commons have some sort of String.isNullOrEmpty or something like that that you could use.
If you don't like either of those options, at least get rid of the extra parentheses. They are not needed here.
---
/*
* Author: Andrew Cathcart, S130315904
* A Vehicle interface
*/
public interface Vehicle {
public String getRegNum();
public int getFuelCapacity();
public int getCurrentFuel();
public void isTankFull();
public boolean isFull();
public boolean isRented();
public void setIsRented(Boolean bool);
public int addFuel(int amount);
public int drive(int distance);
public boolean isSmall();
}
This sort of thing could REALLY use some documentation. You wanted harsh and constructive, and that's what you're gonna get: (some of the critisms may not apply because you're in school)
public interface Vehicle { // A Vehicle interface
As you can see, it says the same as the interface definition, so the comment is useless
public String getRegNum();
Type mismatch, "Num" implies numbers which are not strings, could be really helpful to explain this in documentation. Also, don't shorten function names like this. You're one step away from `getRN`. Reduce typing via autocomplete, not text compression.
public int getFuelCapacity();
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()`.
public int getCurrentFuel();
Same thing here, no unit.
Actually, if you combine the two, they should probably be "current" and "max" fuel. Keep your terms related. And I'd sort them around so that the "current fuel" definition comes before the "max fuel" function definition. That way it reads easier.
public void isTankFull();
Uhh... why doesn't this return a boolean? What is this method gonna do?
public boolean isFull();
... What's the difference between this method and `isTankFull`? The people in the vehicle? Mind you, the entire reason I can get this confused about the requirements is because you put no explanation in code what so ever. This sort of thing is hell to maintenance programmers.
public boolean isRented();
Ohh, vehicles can be rented!
public void setIsRented(Boolean bool);
This shouldn't be a `Boolean`, it should be a `boolean`. Else I'll toss `null` in. And bad things will happen.
public int addFuel(int amount);
I know you're likely to add fuel to a vehicle in real life, but tracking applications usually don't do all the things that you'd do in real life. They just track the current status - which is likely to deviate from estimations, so `setFuel` would probably be better. ... That is to say, I'm missing a setter. `addFuel` is good, but I'm missing a setter.
By the way, what is that int return type? Overflow?
public int drive(int distance);
distance goes in! Numbers come out! It makes no sense! Also, are those meters, kilometers, miles, yards, feet, inches...
public boolean isSmall();
This is a possible design flaw, but understandable, I think. I'd love to talk about it some more, but the other issues are far more important. I wonder why being small matters, though.
}
---
In an interface you NEED to define the semantics. If you define the semantics in function names and argument names then it is much easier to verify implementations.
---
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.