Refactoring large conditional method using method references

Some years ago I wrote junit-parameters, which is basically a custom JUnit test runner that make it possible to add parameters to JUnit 4 test methods.

Browsing its source code SonarLint pointed me a large conditional if/else method from the ParameterTypeConverterFactory class:

This method converts the method parameter to its specific type based on its Class object. As it is few lines long, it showed me a good opportunity to refactor this code a little bit with a more elegant solution. This project has unit tests, so I could start refactoring it in small steps and start over again whether I have made a mistake.

I started by defining a functional interface called ParameterConverter:

and I created an immutable map which maps each Class object to its associated ParameterConverter instance (making use of method references):

Then I refactored the original conditional method to get the ParameterConverter instance from the convertersByClass map and mapping to an Optional instance in case it didn’t exist.

After those refactorings, SonarLint stopped warning me. Below is the modified version of the original method with some helper methods:

The complete code can be found at my GitHub here.

This was the first change of breaking this complicated conditional method into a more readable one. It was safe to do so because after each change I could run my unit tests suite to assert I haven’t broken anything. The next refactoring could be towards a more object-oriented approach like Replace Conditional with Polymorphism.

What did you think about this refactoring? Have you ever had such a situation? Drop your comments here!

3 Things Every Java Developer Should Stop Doing

My friends Andre and Leonnardo have sent me an interesting article about some bad habits every Java developer should stop doing in their code.

Basically the author picked up the following points and discussed each one of them, showing code examples why they aren’t good practices at all:

  1. Returning Null
  2. Defaulting to Functional Programming
  3. Creating Indiscriminate Getters and Setters

I agree with all these points as being bad practices in Java code. How about you? What do you think about it?

Java 8: Converting Optional Collection to the Streams API

Although Java 9 has already been released, this post is about converting an optional collection to the Streams API introduced in Java 8.

Suppose some person could have zero, one or more cars and it is represented by the Person class below (some code omitted).

public class Person {

    private String name;

    .
    .
    .

    public Optional> getCars() {
        return Optional.ofNullable(cars);
    }

    .
    .
    .

}

Now we create a list of people and we want to get Mark’s cars.

Person mark = new Person("Mark");

List people = ...

How can we do that using the Streams API, since the getCars() method return an Optional?

One possibility is to filter people’s list by Mark’s name, filter the Optional if it is present or not and map its wrapped value (our cars list):

Collection markCars = people
                .stream()
                .filter(person -> "Mark".equals(person.getName()))
                .findFirst()
                .map(Person::getCars)
                .filter(Optional::isPresent)
                .map(Optional::get)
                .orElse(Collections.emptyList());

At this moment we reached the reason of this blog post. And how can we get all people’s cars? The idea here is to use the flatMap() operation unwrapping the Optional to the collection’s stream when it is present or getting an empty stream when it isn’t present:

Collection allPeopleCars = people
                .stream()
                .map(Person::getCars)
                .flatMap(mayHaveCars -> mayHaveCars.isPresent() ? mayHaveCars.get().stream() : Stream.empty())
                .collect(Collectors.toList());

We can do better and replace the above solution to be more functional using method references:

Collection allPeopleCars = people
                .stream()
                .map(Person::getCars)
                .flatMap(mayHaveCars -> mayHaveCars.map(Collection::stream).orElse(Stream.empty()))
                .collect(Collectors.toList());

If you use IntelliJ IDEA as your favorite IDE, it has an inspection that helps replacing Optional.isPresent() with a functional expression:

Collection allPeopleCars = people
                .stream()
                .map(Person::getCars)
                .flatMap(mayHaveCars -> mayHaveCars.map(Collection::stream).orElseGet(Stream::empty))
                .collect(Collectors.toList());

P.S. In Java 9, the stream() method was added to the Optional API, so we can rewrite the above stream pipeline into the following one:

Collection allPeopleCars = people
                .stream()
                .map(Person::getCars)
                .flatMap(Optional::stream)
                .flatMap(Collection::stream)
                .collect(Collectors.toList());

In case you are interested, this post on the IntelliJ IDEA blog has some good tips when working with Java 8.

JavaCodeKata – Lambdas

I’ve came across that kata from @brjavaman and @yanaga to teach lambdas, one of the new features of JDK 8.

There are some unit tests to validate the solution. I’ve found it a good opportunity to exercise the use of lambdas so I decided to solve it. Below is my solution to this kata.

The first method should take the String list and sort all the String elements in ascending (ASCII) order:

    /**
     * This method should take the String List and sort all the String elements in ascending (ASCII) order.
     *
     * @return The sorted values in ascending ASCII order.
     */
    public List getSortedStrings() {
        return values
                .stream()
                .sorted()
                .collect(Collectors.toList());
    }

The other method should take the String list and:

  1. filter the elements that contains one or more digits
  2. transform (map) the remaining Strings into Integers
  3. sort the Integers in ascending order
        
    /**
     * This method should take the String List and:
     * 
    *
  1. filter the elements that contains one or more digits;
  2. *
  3. transform (map) the remaining Strings into Integers;
  4. *
  5. sort the Integers in ascending order.
  6. *
* * @return */ public List getSortedIntegers() { return values .stream() .filter(s -> s.matches("\\d+")) .map(Integer::valueOf) .sorted() .collect(Collectors.toList()); }

The last method should take the String list and:

  1. filter the elements that contains one or more digits
  2. transform (map) the remaining Strings into Integers
  3. sort the Integers in descending order
    
    /**
     * This method should take the String List and:
     * 
    *
  1. filter the elements that contains one or more digits;
  2. *
  3. transform (map) the remaining Strings into Integers;
  4. *
  5. sort the Integers in descending order.
  6. *
* * @return */ public List getSortedDescendingIntegers() { return values .stream() .filter(s -> s.matches("\\d+")) .map(Integer::valueOf) .sorted(Comparator.reverseOrder()) .collect(Collectors.toList()); }

Note that the steps filter the elements that contains one or more digits and transform (map) the remaining Strings into Integers are identical. So I decided to extract the partialĀ Stream into a method with the Extract Method refactoring support on IntelliJ IDEA:

    private Stream integersWithOneOrMoreDigits() {
        return values
                .stream()
                .filter(s -> s.matches("\\d+"))
                .map(Integer::valueOf);
    }

Then I refactored the the solution to use the new extracted method:

    
    public List getSortedIntegers() {
        return integersWithOneOrMoreDigits()
                .sorted()
                .collect(Collectors.toList());
    }
    public List getSortedDescendingIntegers() {
        return integersWithOneOrMoreDigits()
                .sorted(Comparator.reverseOrder())
                .collect(Collectors.toList());
    }

I re-run the tests and they all passed. What do you think about this solution? Do you suggest other ones?