A Car Rental Agency - guaranteeing uniqueness

Question

Just looking for some constructive (harsh) criticism of a project I've completed and handed in. This is a theoretical implementation of the system, specifically has a simplified registration number and a simplified driving licence number generator. I've added in the other classes for clarity (and criticism is welcome for those) but would like focus on the RegistrationNumber.java class and LicenceNumber.java class and if I have guaranteed uniqueness.

---

RentalAgency.java

package carhireapp;

import java.util.*;

/*
 * Author: Andrew Cathcart, S130315904
 * Main rental agency class
 * Contains the companies fleet of cars that they rent, as well as methods to get the currently 
 * rented cars, get the number of available cars of a certain size, see what car a certain 
 * driving licence is renting, issue a car to an individual with a valid licence and also terminate a rental.
 */
public class RentalAgency {
    private static List<Vehicle> ListOfCars = new ArrayList<Vehicle>();
    private static Map<DrivingLicence, Vehicle> FLEET = new HashMap<DrivingLicence, Vehicle>();

    // When RentalAgency is created, populate the ListOfCars
    public RentalAgency() {
        populateList();
    }

    // A method to populate the map of vehicles with 20 small cars and 10 large
    // cars
    private void populateList() {
        for (int i = 0; i < 20; i++) {
            ListOfCars.add(new SmallCar());
        }

        for (int i = 0; i < 10; i++) {
            ListOfCars.add(new LargeCar());
        }
    }

    // Returns the entire List listOfCars
    public List<Vehicle> getListOfCars() {
        return ListOfCars;
    }

    // Returns the entire map FLEET
    public Map<DrivingLicence, Vehicle> getFleet() {
        return FLEET;
    }

    /*
     * True for small, false for large. For all objects in the list, if the
     * vehicle in the list is a SmallCar object and is not rented, add to the
     * counter
     */
    public int availableCars(Boolean isSmall) {
        int count = 0;
        for (Vehicle temp : ListOfCars) {
            if (temp.isSmall() == isSmall)
                if (!temp.isRented()) {
                    count++;
                } else if (!temp.isRented()) {
                    count++;
                }
        }
        return count;
    }

    // Returns a list of vehicle objects that are currently rented
    public List<Vehicle> getRentedCars() {
        List<Vehicle> rentedCars = new ArrayList<Vehicle>();
        for (Vehicle temp : ListOfCars) {
            if (temp.isRented()) {
                rentedCars.add(temp);
            }
        }
        return rentedCars;
    }

    // Returns the car matching a driving licence
    public Vehicle getCar(DrivingLicence licence) {
        if (FLEET.containsKey(licence)) {
            return FLEET.get(licence);
        } else
            return null;
    }

    public void issueCar(DrivingLicence licence, Boolean isSmall) {
        Calendar dob = Calendar.getInstance();
        dob.setTime(licence.getDriverDateOfBirth());
        Calendar today = Calendar.getInstance();
        int age = today.get(Calendar.YEAR) - dob.get(Calendar.YEAR);

        if (today.get(Calendar.MONTH) < dob.get(Calendar.MONTH)) {
            age--;
        } else if (today.get(Calendar.MONTH) == dob.get(Calendar.MONTH)
                && today.get(Calendar.DAY_OF_MONTH) < dob.get(Calendar.DAY_OF_MONTH)) {
            age--;
        }

        Calendar doi = Calendar.getInstance();
        doi.setTime(licence.getDateOfIssue());
        int yearsHeld = today.get(Calendar.YEAR) - doi.get(Calendar.YEAR);
        if (today.get(Calendar.MONTH) < doi.get(Calendar.MONTH)) {
            yearsHeld--;
        } else if (today.get(Calendar.MONTH) == doi.get(Calendar.MONTH)
                && today.get(Calendar.DAY_OF_MONTH) < doi.get(Calendar.DAY_OF_MONTH)) {
            yearsHeld--;
        }
        /*
         * Code to calculate the age of the person and also how many years
         * they've held their licence Credited to user Zds from
         * stackoverflow.com and Irene Loos from coderanch.com
         * http://www.coderanch.com/t/391834/java/java/calculate-age
         * http://stackoverflow.com/questions/1116123/how-do-i-calculate-
         * someones-age-in-java
         */

        boolean flag = false;
        // A simple flag to toggle depending on if we find an appropriate car to
        // issue
        if ((licence.isFull()) && (!licence.getCurrentlyRenting())) {
            // If the individual has a full licence and is not currently renting
            // a car
            for (Vehicle temp : ListOfCars) {
                // iterates through the list of Vehicles
                if (temp.isSmall() == isSmall) {
                    // checks if the user entered true or false for isSmall and
                    // finds cars in the list from this
                    if ((age >= 21) && (yearsHeld >= 1)) {
                        // checks their current age and how many years they've
                        // owned their licence against the requirements
                        if ((!temp.isRented()) && (temp.isFull())) {
                            // It then checks that the car in the list is not
                            // rented and has a full tank
                            temp.setIsRented(true);
                            licence.setCurrentlyRenting(true);
                            FLEET.put(licence, temp);
                            flag = false;
                            break;
                        } else if ((age >= 25) && (yearsHeld >= 5) && (!temp.isRented()) && (temp.isFull())) {
                            temp.setIsRented(true);
                            licence.setCurrentlyRenting(true);
                            FLEET.put(licence, temp);
                            flag = false;
                            break;
                        } else
                            flag = true;
                    } else
                        flag = true;
                } else
                    flag = true;
            }
        } else
            flag = true;
        if (flag) {
            System.out.println("An appropriate car could not be issued");
        }
    }

    // Removes key:value pairs from a map when given a licence object
    // Also sets DrivingLicence's currentlyRenting status to false and Vehicle's
    // isRented status to false
    // Returns the fuel required to fill the tank, else -1
    public int terminateRental(DrivingLicence licence) {
        if (FLEET.containsKey(licence)) {
            int fuelRequiredToFill = ((FLEET.get(licence).getFuelCapacity()) - (FLEET.get(licence).getCurrentFuel()));
            licence.setCurrentlyRenting(false);
            FLEET.get(licence).setIsRented(false);
            FLEET.remove(licence);
            return fuelRequiredToFill;
        }
        return -1;
    }
}

---

Vehicle.java

package carhireapp;

/* 
 * 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();

}

AbstractVehicle.java

package carhireapp;

/* 
 * Author: Andrew Cathcart, S130315904
 * An Abstract class which implements the Vehicle interface
 * This class does not include implementation for the drive method in the Vehicle interface
 */
public abstract class AbstractVehicle implements Vehicle {
    private RegistrationNumber regNum;
    private int fuelCapacity;
    private int currentFuel;
    private boolean isFull;
    private boolean isRented;

    public AbstractVehicle() {
        RegistrationNumber regNumObj = RegistrationNumber.getInstance();
        regNum = regNumObj;
        isFull = true;
        setIsRented(false);
    }

    public String getRegNum() {
        return regNum.getStringRep();
    }

    public void setFuelCapacity(int capacity) {
        this.fuelCapacity = capacity;
    }

    public int getFuelCapacity() {
        return fuelCapacity;
    }

    public int getCurrentFuel() {
        return currentFuel;
    }

    public void setCurrentFuel(int amount) {
        currentFuel = amount;
        isTankFull();
    }

    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;
    }

    public boolean isRented() {
        return isRented;
    }

    public void setIsRented(Boolean bool) {
        isRented = bool;
    }

    public int addFuel(int amount) {
        if (amount <= 0) {
            throw new IllegalArgumentException("You must add an amount greater than zero");
        }

        if (isFull || !isRented) {
            return 0;
        }
        // If the tank is full or the car is not rented return zero

        if ((currentFuel + amount) <= fuelCapacity) {
            currentFuel += amount;
            if (currentFuel == fuelCapacity) {
                isFull = true;
                return amount;
            } else
                return amount;
        }
        // If the current fuel plus the amount to add is less than or equal to
        // the fuel capacity, add the amount to the current fuel and if the
        // current fuel is equal to the fuel capacity then set the boolean
        // isFull to true and return the amount added

        if ((currentFuel + amount) > fuelCapacity) {
            int difference = (fuelCapacity - currentFuel);
            currentFuel = fuelCapacity;
            isFull = true;
            return difference;
        }
        // Covers the case where the amount added would cause the current fuel
        // to exceed the fuel capacity

        return -1;
    }
}

SmallCar.java

package carhireapp;
/* 
 * Author: Andrew Cathcart, S130315904
 * Implements the drive method for a small car
 * Super class is AbstractVehicle
 */
public class SmallCar extends AbstractVehicle {
private int smallFuelCapacity = 45;
        private boolean isSmall = true;

        // Calls the super constructor, sets the fields appropriately
        public SmallCar() {
            super();
            super.setFuelCapacity(smallFuelCapacity);
            super.setCurrentFuel(smallFuelCapacity);
        }

        public boolean isSmall() {
            return isSmall;
        }

        // 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;
        }


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

LargeCar.java

package carhireapp;

/* 
 * Author: Andrew Cathcart, S130315904
 * Implements the drive method for a large car
 * Super class is AbstractVehicle
 */
public class LargeCar extends AbstractVehicle {
    private int largeFuelCapacity = 65;
    private boolean isSmall = false;

    // Calls the super constructor, sets the fields appropriately
    public LargeCar() {
        super();
        super.setFuelCapacity(largeFuelCapacity);
        super.setCurrentFuel(largeFuelCapacity);
    }

    public boolean isSmall() {
        return isSmall;
    }

    // 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)) {
            if (distance <= 50) {
                fuelUsed = (distance / 15);
                super.setCurrentFuel(super.getCurrentFuel() - fuelUsed);
                return fuelUsed;
            } else {
                int moreThan = (distance - 50);
                fuelUsed = (50 / 15) + (moreThan / 20);
                super.setCurrentFuel(super.getCurrentFuel() - fuelUsed);
                return fuelUsed;
            }
        }

        return fuelUsed;
    }


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

---

Implementation - Driving Licence

Car Registration Number

For this project (though not in real life) a car registration number has two components - a single letter followed by a four digit number. For example: - a1234 You must provide access to each component and an appropriate string representation of the registration number.

Registration numbers are unique. You must guarantee that no two cars have the same registration number.

RegistrationNumber.java

package carhireapp;

import java.util.HashMap;
import java.util.Map;
import java.util.Random;

public final class RegistrationNumber {
    private static final Map<String, RegistrationNumber> REGNUM = new HashMap<String, RegistrationNumber>();
    // Stores stringRep with object
    private final char letter; // One letter
    private final int numbers; // Four numbers
    private final String stringRep; // letter + number, e.g. A1234

    private RegistrationNumber(char letter, int numbers) {
        this.letter = letter;
        this.numbers = numbers;
        this.stringRep = String.format("%s%04d", letter, numbers);
        // Pad the string to make sure we always get a four digit number
    }

    public static RegistrationNumber getInstance() {
        Random random = new Random();
        // Using the random class instead of math.random as it is a static
        // method
        final Character letter = (char) (random.nextInt(26) + 'A');
        final int numbers = random.nextInt(9000) + 1000;
        final String stringRep = letter + numbers + "";

        if (!REGNUM.containsKey(stringRep)) {
            REGNUM.put(stringRep, new RegistrationNumber(letter, numbers));
        }
        // If the randomly generated registration plate is unique then create a
        // new object and return a reference to it
        else if (REGNUM.containsKey(stringRep)) {
            return getInstance();
        }
        // If the randomly generated registration plate is not unique, call the
        // getInstance method again

        return REGNUM.get(stringRep);
        // return a reference
    }

    public char getLetter() {
        return letter;
    }

    public int getNumbers() {
        return numbers;
    }

    public String getStringRep() {
        return stringRep;
    }

    public String toString() {
        return "RegistrationNumber [letter=" + letter + ", numbers=" + numbers + ", stringRep=" + stringRep + "]";
    }
}

Driving Licence

You must guarantee the uniqueness of licence numbers.

LicenceNumber.java

package carhireapp;

import java.util.Date;
import java.util.HashMap;
import java.util.Map;
import java.util.Random;
import java.util.Calendar;

public final class LicenceNumber {
    private static final Map<String, LicenceNumber> LICENCENUM = new HashMap<String, LicenceNumber>();
    private final String initials;
    private final int yearOfIssue;
    private final int serialNum;
    private final String stringRep;

    private LicenceNumber(String initials, int yearOfIssue2, int serialNum) {
        this.initials = initials;
        this.yearOfIssue = yearOfIssue2;
        this.serialNum = serialNum;
        stringRep = initials + "-" + yearOfIssue2 + "-" + serialNum;
    }

    public static LicenceNumber getInstance(Name fullName, Date dateOfIssue) {
        final String initials = fullName.getFirstName().substring(0, 1) + fullName.getLastName().substring(0, 1);
        Calendar cal = Calendar.getInstance();
        cal.setTime(dateOfIssue);
        final int yearOfIssue = cal.get(Calendar.YEAR);
        Random r = new Random();
        // Using the random class instead of math.random as it is a static
        // method
        final int serialNum = r.nextInt(11);
        final String stringRep = initials + "-" + yearOfIssue + "-" + serialNum;

        if (!LICENCENUM.containsKey(stringRep)) {
            LICENCENUM.put(stringRep, new LicenceNumber(initials, yearOfIssue, serialNum));
        } else if (LICENCENUM.containsKey(stringRep)) {
            return getInstance(fullName, dateOfIssue);
        }
        // If two people have the same name, date of birth and are generated the
        // same serial number, call the getInstance again

        return LICENCENUM.get(stringRep);
        // If the licence number is unique then create a
        // new object, put it into the HashMap and return a reference to it,
        // else return a reference
    }

    public String getInitials() {
        return initials;
    }

    public int getYearOfIssue() {
        return yearOfIssue;
    }

    public int getSerialNum() {
        return serialNum;
    }

    public String getStringRep() {
        return stringRep;
    }

    @Override
    public String toString() {
        return "LicenceNumber [initials=" + initials + ", yearOfIssue=" + yearOfIssue + ", serialNum=" + serialNum
                + ", stringRep=" + stringRep + "]";
    }
}

Name.java

package carhireapp;

/*
 * Author: Andrew Cathcart, S130315904
 * Relied upon by LicenceNumber class
 * A simple class used to create and store information about a persons name
 */
public final class Name {

    private final String firstName;
    private final String lastName;

    public Name(String firstName, String lastName) {
        if ((firstName == null) || (firstName.isEmpty())) {
            throw new IllegalArgumentException("firstName cannot be null or empty");
        }
        if ((lastName == null) || (lastName.isEmpty())) {
            throw new IllegalArgumentException("lastName cannot be null or empty");
        }
        this.firstName = firstName;
        this.lastName = lastName;
    }

    public String getFirstName() {
        return firstName;
    }

    public String getLastName() {
        return lastName;
    }

    @Override
    public String toString() {
        return "firstName= " + firstName + " lastName= " + lastName;
    }
}

DrivingLicence.java

package carhireapp;

import java.util.Date;

/*
 * Author: Andrew Cathcart, S130315904
 * Relies on the Name and LicenceNumber classes
 * A simple class to store information about a driving licence
 */
public final class DrivingLicence {
    private final Name driverName;
    private final Date driverDateOfBirth;
    private final Date dateOfIssue;
    private final LicenceNumber number;
    private final boolean isFull;
    private boolean currentlyRenting = false;

    public DrivingLicence(Name driverName, Date dateOfBirth, Date dateOfIssue, boolean isFull) {
        this.driverName = driverName;
        this.driverDateOfBirth = dateOfBirth;
        this.dateOfIssue = dateOfIssue;
        this.number = LicenceNumber.getInstance(driverName, dateOfIssue);
        this.isFull = isFull;
    }

    public Name getDriverName() {
        return driverName;
    }

    public Date getDriverDateOfBirth() {
        return driverDateOfBirth;
    }

    public Date getDateOfIssue() {
        return dateOfIssue;
    }

    public LicenceNumber getNumber() {
        return number;
    }

    public boolean isFull() {
        return isFull;
    }

    public void setCurrentlyRenting(Boolean bool) {
        currentlyRenting = bool;
    }

    public boolean getCurrentlyRenting() {
        return currentlyRenting;
    }

    @Override
    public String toString() {
        return "\nDrivingLicence \ndriverName= " + driverName.toString() + "\ndriverDateOfBirth= " + driverDateOfBirth
                + "\ndateOfIssue= " + dateOfIssue + "\nnumber= " + number.toString() + "\nisFull= " + isFull + "]";
    }

}

Answer 1

RentalAgency.java

    private static List<Vehicle> ListOfCars = new ArrayList<Vehicle>();

The standard for Java variable naming is camelCase, not PascalCase. So listOfCars. And personally, I'd probably just name it cars. Or make that one the fleet variable. Then if you want to change this from a List to a Set, you don't have to change the name as well.

I also wouldn't make this static. By making this static, you are making it so that every rental agency shares the same list. It seems more like each rental agency should have its own list of vehicles.

    private static Map<DrivingLicence, Vehicle> FLEET = new HashMap<DrivingLicence, Vehicle>();

So this says that you can only rent one vehicle per driver's license. While renting multiple vehicles is rare, it's not unknown. And if you include trailers as separate vehicles, it's not even that rare. Making it static exacerbates this.

I might call this rentedCars, since that's what it holds. FLEET doesn't give us any notion of that.

        for (int i = 0; i < 20; i++) {

Consider using constants for this. Or pass it as a parameter into the method.

    public int availableCars(Boolean isSmall) {

I would expect a method named availableCars to return a Collection of available cars. Methods are normally given verb names, as they do things. In this case, countAvailableCars.

        for (Vehicle temp : ListOfCars) {

What's any more temporary about this than any other variable? Why not just name it car?

            if (temp.isSmall() == isSmall)
                if (!temp.isRented()) {
                    count++;
                } else if (!temp.isRented()) {
                    count++;
                }

The else if doesn't do anything.

            if (car.isSmall() == small) {
                if (!car.isRented()) {
                    count++;
                }
            }

So just leave it off.

Even if there is only one statement, it is often easier to include the {} of the block form. That makes it easier to see what statement goes with what, and it makes it easier to add additional statements later.

Variables are generally given noun names, not verb names. An adjective like small is closer than isSmall.

Consider storing small and large cars in separate variables. Then you wouldn't have to count them each time.

    public List<Vehicle> getRentedCars() {
        List<Vehicle> rentedCars = new ArrayList<Vehicle>();
        for (Vehicle temp : ListOfCars) {
            if (temp.isRented()) {
                rentedCars.add(temp);
            }
        }
        return rentedCars;
    }

This is more complicated than it needs to be

    public List<Vehicle> getRentedCars() {
        return new ArrayList<Vehicle>(FLEET.values());
    }

Since FLEET already lists all the rented cars.

We can do even better if we change the return type to Collection<Vehicle>, because then we don't have to create the list.

        if (FLEET.containsKey(licence)) {
            return FLEET.get(licence);
        } else
            return null;

If a Map doesn't contain a a particular key, get returns null. So

        return FLEET.get(licence);

is sufficient to get the same result.

        Calendar dob = Calendar.getInstance();
        dob.setTime(licence.getDriverDateOfBirth());
        Calendar today = Calendar.getInstance();
        int age = today.get(Calendar.YEAR) - dob.get(Calendar.YEAR);

        if (today.get(Calendar.MONTH) < dob.get(Calendar.MONTH)) {
            age--;
        } else if (today.get(Calendar.MONTH) == dob.get(Calendar.MONTH)
                && today.get(Calendar.DAY_OF_MONTH) < dob.get(Calendar.DAY_OF_MONTH)) {
            age--;
        }

        Calendar doi = Calendar.getInstance();
        doi.setTime(licence.getDateOfIssue());
        int yearsHeld = today.get(Calendar.YEAR) - doi.get(Calendar.YEAR);
        if (today.get(Calendar.MONTH) < doi.get(Calendar.MONTH)) {
            yearsHeld--;
        } else if (today.get(Calendar.MONTH) == doi.get(Calendar.MONTH)
                && today.get(Calendar.DAY_OF_MONTH) < doi.get(Calendar.DAY_OF_MONTH)) {
            yearsHeld--;
        }

Both sections perform the same computation, so you could simplify this with a method.

    public static int subtractFromToday(Date date) {
        Calendar today = Calendar.getInstance();
        Calendar calendar = Calendar.getInstance();
        calendar.setTime(date);

        int yearDifference = today.get(Calendar.YEAR) - calendar.get(Calendar.YEAR);    
        if (today.get(Calendar.MONTH) < calendar.get(Calendar.MONTH)) {
            yearDifference--;
        } else if (today.get(Calendar.MONTH) == calendar.get(Calendar.MONTH)
                && today.get(Calendar.DAY_OF_MONTH) < calendar.get(Calendar.DAY_OF_MONTH)) {
            yearDifference--;
        }

        return yearDifference;
    }

and use it like

        int age = subtractFromToday(licence.getDriverDateOfBirth());
        int yearsHeld = subtractFromToday(licence.getDateOfIssue());

This would also reduce the length of your issueCar method, which is rather long.

                if (temp.isSmall() == isSmall) {

and

                } else
                    flag = true;

This is just wrong. You are setting flag to true if a single car does not match the small status requested.

                    if ((age >= 21) && (yearsHeld >= 1)) {

Why do you check this for every car? You could just say something like

        if (age < 21) {
            System.out.println("You aren't old enough to rent a car.");
            return;
        }

        if (yearsHeld < 1) {
            System.out.println("You haven't been licenced long enough to rent a car.");
            return;
        }

before checking for available cars.

                        if ((!temp.isRented()) && (temp.isFull())) {
                            // It then checks that the car in the list is not
                            // rented and has a full tank
                            temp.setIsRented(true);
                            licence.setCurrentlyRenting(true);
                            FLEET.put(licence, temp);
                            flag = false;
                            break;
                        } else if ((age >= 25) && (yearsHeld >= 5) && (!temp.isRented()) && (temp.isFull())) {
                            temp.setIsRented(true);
                            licence.setCurrentlyRenting(true);
                            FLEET.put(licence, temp);
                            flag = false;
                            break;
                        } else
                            flag = true;

Again, the else if will never be true. The last two conditions are always false if you take the else case. So that can be removed.

                    if ((!temp.isRented()) && (temp.isFull())) {
                        // It then checks that the car in the list is not
                        // rented and has a full tank
                        temp.setIsRented(true);
                        licence.setCurrentlyRenting(true);
                        FLEET.put(licence, temp);
                        return;
                    }

Returning instead of breaking allows us to get rid of the flag variable.

        if (flag) {
            System.out.println("An appropriate car could not be issued");
        }

can become

        System.out.println("An appropriate car could not be issued");

without any gating.

            FLEET.get(licence).setIsRented(false);
            FLEET.remove(licence);

You can simplify this to

            FLEET.remove(licence).setIsRented(false);

The remove method returns the same value as get.

RegistrationNumber.java

        Random random = new Random();
        // Using the random class instead of math.random as it is a static
        // method

Math.random handles the initialization of the random object for you. It saves having to instantiate the Random object if you never call the getInstance() method.

If you don't care about that, you could create a class field like

    private static final Random RANDOM = new Random();

This saves having to create a new one for each call to getInstance.

        if (!REGNUM.containsKey(stringRep)) {
            REGNUM.put(stringRep, new RegistrationNumber(letter, numbers));
        }
        // If the randomly generated registration plate is unique then create a
        // new object and return a reference to it
        else if (REGNUM.containsKey(stringRep)) {
            return getInstance();
        }
        // If the randomly generated registration plate is not unique, call the
        // getInstance method again

Elsewhere you put your } and else on the same line. If that's how you want to do it, then do it everywhere. And I would always avoid putting comments in between an if block and an else, as it makes it hard to see that the else exists. The presumption is that the } ends the statement unless there is an immediate else.

And I'd write this particular case differently anyway.

        // If the randomly generated registration plate has already been used, 
        if (REGNUM.containsKey(stringRep)) {
            // call the getInstance method again
            return getInstance();
        }

        // Otherwise create a new object
        REGNUM.put(stringRep, new RegistrationNumber(letter, numbers));

If you are returning, you don't need an else. Also, I would prefer to check if something is true rather than not true.

If this is a single-threaded application, this is enough to ensure that there are no duplicates. If multi-threaded, then you need some way to ensure that two threads don't do their containsKey checks at the same time.

Note that as the number of registrations increase, this method's performance will get worse and worse. Once there are 234,000 registrations, it will just loop forever. Perhaps this is why registrations tend to be issued sequentially instead.

LicenceNumber.java

    public static LicenceNumber getInstance(Name fullName, Date dateOfIssue) {
        final String initials = fullName.getFirstName().substring(0, 1) + fullName.getLastName().substring(0, 1);
        Calendar cal = Calendar.getInstance();
        cal.setTime(dateOfIssue);
        final int yearOfIssue = cal.get(Calendar.YEAR);
        Random r = new Random();
        // Using the random class instead of math.random as it is a static
        // method
        final int serialNum = r.nextInt(11);
        final String stringRep = initials + "-" + yearOfIssue + "-" + serialNum;

        if (!LICENCENUM.containsKey(stringRep)) {
            LICENCENUM.put(stringRep, new LicenceNumber(initials, yearOfIssue, serialNum));
        } else if (LICENCENUM.containsKey(stringRep)) {
            return getInstance(fullName, dateOfIssue);
        }
        // If two people have the same name, date of birth and are generated the
        // same serial number, call the getInstance again

        return LICENCENUM.get(stringRep);
        // If the licence number is unique then create a
        // new object, put it into the HashMap and return a reference to it,
        // else return a reference
    }

I don't like the repeated effort at the beginning. Consider using these two methods instead

    public static LicenceNumber getInstance(Name fullName, Date dateOfIssue) {
        final String initials = fullName.getFirstName().substring(0, 1) + fullName.getLastName().substring(0, 1);

        Calendar cal = Calendar.getInstance();
        cal.setTime(dateOfIssue);

        return getInstance(initials, cal.get(Calendar.YEAR));
    }

    private static LicenceNumber getInstance(String initials, int yearOfIssue) {
        final int serialNum = RANDOM.nextInt(11);
        final String stringRep = initials + "-" + yearOfIssue + "-" + serialNum;

        if (LICENCENUM.containsKey(stringRep)) {
        // If two people have the same name, date of birth and are generated the
        // same serial number, call the getInstance again
            return getInstance(initials, yearOfIssue);
        }

        LICENCENUM.put(stringRep, new LicenceNumber(initials, yearOfIssue, serialNum));

        return LICENCENUM.get(stringRep);
    }

Now you only calculate the initials and yearOfIssue once regardless of how many attempts you make.

Again, this will offer declining performance as the slots fill and will loop forever once they're full.

Answer 2

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 logic 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.

---

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).