3

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()));
Improve this question
14
  • 1
    You can't pass an argument to a method reference. You'll have to use a lambda. Also, don't use of if the value might be null; you have to use ofNullable. Commented May 3, 2017 at 5:28
  • 2
    You should change flatMap to map. Commented May 3, 2017 at 5:28
  • Edited the use. Commented May 3, 2017 at 5:31
  • I assume this still doesn't compile? ifPresent takes a Consumer, which is an interface that takes a value and doesn't return anything. But it has to take either an object that implements Consumer, or a lambda, or a method reference. You haven't given it any of those; you're trying to give it another object. Furthermore, ifPresent returns void so you can't use it in a place that needs a value. Commented May 3, 2017 at 5:36
  • 1
    @dm1530 avoiding abuse Optional, flow control is more sufficient in this case. and loadMatching methods should be belong to VehicleLoader. Commented May 3, 2017 at 5:38

4 Answers 4

Reset to default
4

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

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());
    }
Improve this answer

5 Comments

You could also use Optional::orElseGet, if you wanted to only generate the result of Collections::emptyList if the Optional<> is indeed empty: .orElseGet(Collections::emptyList)
the behavior of your code violate on null checks will result to two methods have different behaviors.
Why is it valid to have the return type List<VehicleMake> even though we are returning Optional.ofNullable(...... Why is it throwing compiler error when I change it to Optional<List<VehicleMake>> ?
@holi-java Would the different behavior be anything else except this new method not throwing any IllegalArgumentException as compared to the previous one which does so?
@dm1530 not only haven't throws IllegalArgumentException, but also return an empty list instead of null, when makes return by getVehicleMakesByRegion is null.
1

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.

Improve this answer

8 Comments

Isn't the whole point of using Optional to not check for each argument to be null? This code seems to do the same thing but just in Optional way....
@dm1530 well, but the flow control is more clearly than Optional after I edited my answer. Indeed, what you really want is replacing for loop with stream.
Since you are checking for null arguments in the starting of the code, why are you still wrapping the final return statement into Optional.ofNullable ? Is it for the case when loader.getVehicleMakesByRegion(region.name()) returns null?
@dm1530 the Optional.orElse(null) does. and flow control do such thing is sufficient.
Also, the same question that I have for your answer as well. Why is it valid to have the return type of the method as List<VehicleMake> even though you are returning Optional.ofNullable(...... Why does it throw compiler error when I change it to Optional<List<VehicleMake>> ?
|
0

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"));
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.