Hacker News new | ask | show | jobs
by rakpol 3647 days ago
As someone who could very easily be on the other side of that code review (and I'm preeeettty sure I'm not in this case?) I feel obliged to at least try to provide a counterpoint :).

So I agree that enormous blobs of unreadable crap are indeed unreadable, and that regardless of how neat and functional your code is, it can still be complete gibberish to most people. That being said, long chains of streams can be broken out quite nicely by using intermediate names:

e.g.

  Comparator<Transaction> descendingTransactionsByValue = comparing(Transaction::getValue).reversed();
  Stream<Transaction> groceries = transactions.filter(t -> t.getType() == Transaction.GROCERY);
  Stream<Transaction> sortedGroceries = groceries.sorted(descendingTransactionsByValue);
  Stream<Transaction> transactionids = sortedGroceries.map(Transaction::getId).collect(toList());
vs. the first code block under "Figure 1" in the linked article.

For me at least, it helps keep code from getting too unruly: you only have one 'thing' you can do in a filter, map or sorted call, unlike in a foreach loop where anything can go. So my thesis is this: using streams I can quickly scan over the function/stream names to get the gist of what it's doing, but using foreach loops I need to closely examine each line to have any idea of what's happening :3 (e.g. people love abusing labeled breaks [1] in our codebase, as well as excessively modifying input parameters w/o documentation, so I might be a bit biased against for loops)

[1]: https://stackoverflow.com/questions/14960419/is-using-a-labe...

2 comments

I find the code in the linked article much more readable than that. You've introduced a huge amount of noise that makes it hard to see what the actual operations being performed are.
I agree to an extent ... this example is pretty contrived, but when you start getting around ten filter/map/groupby operations in, it gets a little difficult to follow what's supposed to be happening. So typically, my first step towards breaking it out into a method is separating out the individual streams like above. As is mentioned in a cousin comment, it also looks a lot nicer with type inferencing, but alas we are stuck with the verbosity of standard Java 8 for now.
This is a place where local variable type inference really comes in handy for cutting down the noise of the type declarations.

  var descendingTransactionsByValue = comparing(Transaction::getValue).reversed();
  var groceries = transactions.filter(t -> t.getType() == Transaction.GROCERY);
  var sortedGroceries = groceries.sorted(descendingTransactionsByValue);
  var transactionids = sortedGroceries.map(Transaction::getId).collect(toList());
is much easier to understand
Oh definitely, although I'll have to wait for it to be added to the JDK [1] in order to use that in anger :/ (outside of lombok)

[1]: http://openjdk.java.net/jeps/286