Foojay Today

A Minor But Useful Refactoring Technique That Would Reduce Your Code Footprint (Part 2)

October 26, 2022

As I keep refactoring, this article will focus on a few more interesting ways to do it. These are pretty much minor yet effective and useful changes.

Stream.noneMatch() and Stream.anyMatch()

In some situations, we need to find a single case among many. For example, we want to do a certain operation if we have a list of items and a certain item is in that list. Usually, we run a for loop and then use a loop inside the for loop to check the value against our case. Check out the following piece of code:

private void addIfNotExist(final List<GrantedAuthority> reachableRoles,
    final GrantedAuthority authority) {
  for (final GrantedAuthority testAuthority : reachableRoles) {
    final String testKey = testAuthority.getAuthority();
    if ((testKey != null) && (testKey.equals(authority.getAuthority()))) {
      return;
    }
  }
  reachableRoles.add(authority);
}

The above code is easily understandable, which is great, but at the same time, pretty imperative. We are basically instructing our code on every line, what we want and how we want. In many cases, that's fine, but I'd argue that if we can get the same result with a declarative approach, the code will be easier to read and more maintainable. The idea is, let’s not reinvent the wheel. When tools can do a job, we should take the opportunity. Less work is always better. The above code can be refactored as follows:

private void addIfNotExistDeclarative(final List<GrantedAuthority> reachableRoles,
     final GrantedAuthority authority) {
   final var exist = reachableRoles.stream()
       .map(GrantedAuthority::getAuthority)
       .noneMatch(testKey -> (testKey != null) && (testKey.equals(authority.getAuthority())));

   if (exist) {
     reachableRoles.add(authority);
   }
 }

Regarding the number of lines, both approaches have the same number of lines, but I would argue that the second method is preferable because it talks more about its objective when read. When we read code, we mostly want to know the intent behind particular things, but how they were done isn't our main focus.

Let’s see another example-

public boolean isProbablyPlayFramework(String className){
    return className.startsWith("play.api")
        || className.startsWith("play.core")
        || className.startsWith("play.data")
        || className.startsWith("play.filters")
        || className.startsWith("play.libs")
        || className.startsWith("play.mvc")
        || className.startsWith("play.templates");
  }

The above method tests if a fully qualified class name is a part of the play framework or not. Simple, but again, it contains the imperative code. This code can be rewritten as follows -

public boolean isProbablyPlayFramework(String className) {
  return Stream.of("play.api", "play.core", "play.data", "play.filters", "play.libs", "play.mvc",
          "play.templates", "play.utils")
      .anyMatch(className::startsWith);
}

This code certainly reduces the LoC. The other thing we could do is, instead of putting all the class names inside this method, we could extract them and put them in a constant.

This is all for today. In my next article, I'll talk about some small refactoring techniques.

Until then, happy coding.

Topics:

Related Articles

View All

Author(s)

  • A N M Bazlur Rahman

    A N M Bazlur Rahman works at Contrast Security as a Sr. Software Engineer. He has more than ten years of professional experience in the software industry, predominantly in Java ... Learn more

Comments (2)

Your email address will not be published.

Highlight your code snippets using [code lang="language name"] shortcode. Just insert your code between opening and closing tag: [code lang="java"] code [/code]. Or specify another language.

Save my name, email, and website in this browser for the next time I comment.

juwil

talking about readability – ubiquitious null checks should be avoided.

.noneMatch(testKey -> (testKey != null) && (testKey.equals(authority.getAuthority())));
 

would be way better readable as

.noneMatch(authority.getAuthority().equals(testkey));
 
Jamie

I think you forgot to rename the variable in the addIfNotExistDeclarative() function:

Shouldn’t the following:

final var exist = reachableRoles.stream()

be changed instead to something like:

final var noneMatched = reachableRoles.stream()

As it’s supposed to only add to the reachableRoles if it doesn’t already exist?

The imperative version has a guard clause to return; if it is found.

So in that version it only adds the item if it isn’t found.

Subscribe to foojay updates:

https://foojay.io/feed/
Copied to the clipboard