In the spirit of divide and conquer, I’ve gotten into the habit of using helper methods almost everywhere, including for the simplest of tasks.
As a really simple example, let’s say I have a sort of library program written in Java 8 that deals with books, and it has the public method filterByAuthor that takes a list of books and the name of an author as parameters. when designing the implementation, the pseudo code might look something like this:
filter the list of books by
comparing the book's author to the specified one
and when actually implementing the code, I’ll have something like this
public static List<Book> filterByAuthor(List<Book> books, String author) {
return filterBooks(books, book -> isAuthor(book, author));
}
private static List<Book> filterBooks(List<Book> books, Predicate<Book> predicate) {
List<Book> filtered = books.stream()
.filter(predicate)
.collect(Collectors.toList());
return filtered;
}
private static boolean isAuthor(Book book, String author) {
return book.getAuthor().equals(author);
}
In my opinion this is good because it makes the code easier to
- read
- debug
- change
But the same method could easily be written as
public static List<Book>(List<Book> books, String author) {
List<Book> filtered = books.stream()
.filter(book -> book.getAuthor().equals(author))
.collect(Collectors.toList());
return filtered;
}
I’m worried that if this sort of design is used in larger systems it might be overuse of helper methods. (e.g. cluttering the namespace). Are there any disadvantages to this “overuse”?
Helper methods address the two concerns of readability and reusability. Helper methods can make code more readable by encapsulating some logic under a better, more straightforward name. Helper methods can make code more reusable by splitting out a piece of logic that might be reused elsewhere. Through these twin lenses, we can evaluate the value of a helper method.
Your filterBooks method doesn’t add a tremendous amount in terms of additional readability in comparison to the filterByAuthor method. You are still filtering a list of books. It can, however, add value in terms of reusability. If you need to write a filterByPublisher method, for example, you can do that pretty easily and without much duplication.
Your isAuthor method adds value in readability, and may also add value in reusability. Rather than having to parse that a book is authored by someone if the book’s author string-equals the given name, we just read what we intend.
The question of whether each of these provides sufficient value will be dependent on the situation, the standards of the team, and the potential value of readability and reusability. If the helper methods are likely to be reused, the value is there. If the helper methods make maintainability better through simplicity and readability, the value is there.
Methods that don’t add enough value can start to clutter up the code base. There may be more lines of code. The number of methods on a class will be larger, even if many of them are private. When debugging a bit of code, the stack traces will be deeper because they have to descend into all these helper methods, and finding where a particular bit of bad logic is may require more effort. Since many helper methods are private methods on the same class, testing may get a little more complicated as tests will need to meet coverage minimums on the private helpers in addition to the public methods under test.
Helper methods can provide additional benefits beyond just readability and reusability. As we tease out bits of logic, we can see opportunities for refactoring.
For example, the method isAuthor gets my “Tell Don’t Ask” senses tingling. It looks like a method that better belongs on the Book class, say as a method like isAuthoredBy(String author). The filterBooks method looks like it is a method searching for a class to live on. It may make more sense as a filterBy(Predicate<Book> predicate) instance method on a Library or Bookshelf class that encapsulates a collection of Books.
2
Extracting methods is harder than you think it is. It is easy in modern IDEs to have the IDE extract a method. But this does not make it easy to extract a method well.
Firstly, extracting trivial expressions does not necessarily improve readability.
isAuthor(book, author)
book.getAuthor().equals(author)
I think the second one is better. When I look at it, I immediately see what it does. On the other hand, I have to guess at what the isAuthor function is doing. In order for it to make sense to use a function, the functional call should be clearer than the expression, and I don’t think that’s true here.
Secondly, the obvious extraction is not necessarily the correct one. In this case, I think a better way to write your function would be:
public static List<Book> filterByAuthor(List<Book> books, String author) {
return filterBooks(books, Book::getAuthor, author);
}
private static <T> List<Book> filterBooks(List<Book> books, Function<Book, T> getter, T value) {
List<Book> filtered = books.stream()
.filter(book -> getter(book).equals(value))
.collect(Collectors.toList());
return filtered;
}
This extraction manages to pull more of the common filtering logic into the filter function, improving reusability.
Now, you may well disagree with my subjective assessments here. But I think it is true that mindlessly creating helper methods can lead to suboptimal code. Each extraction should be carefully considered, not simply extracted every time there is a non-atomic expression.