Use Java 8 Optional in existing Java 7 code

Question

I have an assignment in which I need to convert the following pre-Java 8 code to Java 8 code. Below is just one method which is giving me hard time to finish up:

  public static List<VehicleMake> loadMatching(Region region, String nameStartsWith, VehicleLoader loader) {
    if ((nameStartsWith == null) || (region == null) || (loader == null)) {
        throw new IllegalArgumentException("The VehicleLoader and both region and nameStartsWith are required when loading VehicleMake matches");
    }
    List<VehicleMake> regionMakes = loader.getVehicleMakesByRegion(region.name());
    if (regionMakes == null) {
        return null;
    }
    List<VehicleMake> matches = new ArrayList<>(regionMakes.size());
    for (VehicleMake make : regionMakes) {
        if ((make.getName() == null) || !make.getName().startsWith(nameStartsWith)) {
            continue;
        }
        matches.add(make);
    }
    return matches;
}

I want to remove the null checks by using Optional<T> without modifying previously created classes and interfaces.

I tried to begin by changing the method return type and doing the following but compiler is throwing this error:
Bad return type in method reference since the VehicleMake class doesn't have optional instance fields.
Following is my code attempt:

   public static Optional<List<VehicleMake>> loadMatchingJava8(Region region, String nameStartsWith, VehicleLoader loader) {
    Optional<List<VehicleMake>> regionMakes = Optional.ofNullable(loader).ifPresent(loader.getVehicleMakesByRegion(Optional.ofNullable(region).ifPresent(region.name())));
    /*

    TODO rest of the conversion
     */
}

EDIT: Removed the flatMap and corrected code by not passing argument to method reference. But now it is not letting me pass region.name() to getVehicleMakesByRegion()

EDIT: Pass in consumer to ifPresent():

Optional<List<VehicleMake>> regionMakes = Optional.ofNullable(loader).ifPresent(()-> loader.getVehicleMakesByRegion(Optional.ofNullable(region).ifPresent(()->region.name()));

Answer 1

You may replace your initial null checks with

Optional.ofNullable(nameStartsWith)
        .flatMap(x -> Optional.ofNullable(region))
        .flatMap(x -> Optional.ofNullable(loader))
        .orElseThrow(() -> new IllegalArgumentException(
            "The VehicleLoader and both region and nameStartsWith"
          + " are required when loading VehicleMake matches"));

but it’s an abuse of that API. Even worse, it wastes resource for the questionable goal of providing a rather meaningless exception in the error case.

Compare with

Objects.requireNonNull(region, "region is null");
Objects.requireNonNull(nameStartsWith, "nameStartsWith is null");
Objects.requireNonNull(loader, "loader is null");

which is concise and will throw an exception with a precise message in the error case. It will be a NullPointerException rather than an IllegalArgumentException, but even that’s a change that will lead to a more precise description of the actual problem.

Regarding the rest of the method, I strongly advice to never let Collections be null in the first place. Then, you don’t have to test the result of getVehicleMakesByRegion for null and won’t return null by yourself.

However, if you have to stay with the original logic, you may achieve it using

return Optional.ofNullable(loader.getVehicleMakesByRegion(region.name()))
               .map(regionMakes -> regionMakes.stream()
                    .filter(make -> Optional.ofNullable(make.getName())
                                            .filter(name->name.startsWith(nameStartsWith))
                                            .isPresent())
                    .collect(Collectors.toList()))
               .orElse(null);

The initial code, which is intended to reject null references, should not get mixed with the actual operation which is intended to handle null references.

Answer 2

I have updated your code with Optional:

     public static List<VehicleMake> loadMatchingJava8(Region region, String nameStartsWith, VehicleLoader loader) {
        Optional<List<VehicleMake>> regionMakes = Optional.ofNullable(region)
                .flatMap(r -> Optional.ofNullable(loader).map(l -> l.getVehicleMakesByRegion(r.name())));

        return Optional.ofNullable(nameStartsWith)
                .map(s -> regionMakes
                    .map(Collection::stream)
                    .orElse(Stream.empty())
                    .filter(make -> make.getName() != null && make.getName().startsWith(s))
                    .collect(Collectors.toList()))
                .orElse(Collections.emptyList());
    }

Answer 3

If you really want to convert flow control to Optional, the code keep consistent with yours should be like this(I break the code in 2 lines for printing):

public static Optional<List<VehicleMake>> loadMatchingJava8(Region region, 
                                                            String nameStartsWith,
                                                            VehicleLoader loader) {
    if ((nameStartsWith == null) || (region == null) || (loader == null)) {
        throw new IllegalArgumentException("The VehicleLoader and both region and " +
                "nameStartsWith are required when loading VehicleMake matches");
    }


    return Optional.ofNullable(loader.getVehicleMakesByRegion(region.name()))
            .map(makers -> makers.stream()
                    .filter((it) -> it.getName() != null
                            && it.getName().startsWith(nameStartsWith))
                    .collect(Collectors.toList()));
}

NOTE: you can see more about why do not abuse Optional in this question.

Answer 4

I can't say this is very elegant, but it should satisfy your requirement. There are no explicit null checks, but it'll throw the exception if any input parameters are null, and it filters out vehicles with invalid names from the resulting list.

public static List<VehicleMake> loadMatching(Region region, String nameStartsWith, VehicleLoader loader) {
    return Optional.ofNullable(nameStartsWith)
            .flatMap(startWith -> Optional.ofNullable(loader)
                    .flatMap(vl -> Optional.ofNullable(region)
                            .map(Region::name)
                            .map(vl::getVehicleMakesByRegion))
                    .map(makes -> makes.stream()
                            .filter(make -> Optional.ofNullable(make.getName())
                                    .filter(name -> name.startsWith(startWith))
                                    .isPresent())
                            .collect(Collectors.toList())))
            .orElseThrow(() -> new IllegalArgumentException("The VehicleLoader and both region and nameStartsWith are required when loading VehicleMake matches"));