A Car Rental Agency - guaranteeing uniqueness

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 ListOfCars = new ArrayList();
private static Map FLEET = new HashMap();

// 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 getListOfCars() {
return ListOfCars;
}

// Returns the entire map FLEET
public Map 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 l

RentalAgency.java

private static List ListOfCars = new ArrayList();

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 FLEET = new HashMap();

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 getRentedCars() {
        List rentedCars = new ArrayList();
        for (Vehicle temp : ListOfCars) {
            if (temp.isRented()) {
                rentedCars.add(temp);
            }
        }
        return rentedCars;
    }

This is more complicated than it needs to be

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

Since FLEET already lists all the rented cars.

We can do even better if we change the return type to Collection, 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

StackExchange Code Review Q#129814, answer score: 5