4

In the comments to an answer of another question, I proposed the following Java code as a better way of writing a more procedural-style variant of the same operation:

employeeService.getEmployee()
    .map(Employee::getId)
    .ifPresent‌​(System.out::println‌​);

(where getEmployee() returns an Optional<Employee>)

An upvoted response to this comment suggests that this "drastically reduces code readability" and that the commentor would reject it in code review.

I don't understand this position: to me the code seems as readable, or even more readable, than the alternative procedural style code it was intended to replace. And yet at least two viewers of that comment believe it to be hard to read. Why is that? What makes this code difficult to read (at least for some readers)?

Jules
  • 17,614
  • 2
  • 33
  • 63
  • 11
    That's just one opinion, and it's been expressed on a post that discusses a dubious technique. I think you can safely ignore it and use your own best judgement for readability. – Robert Harvey Jan 20 '18 at 17:29
  • 3
    Because Java programmers have been behind the times. They'll catch up and embrace this, soon enough. – Alexander Jan 20 '18 at 22:04

4 Answers4

11

Everyone talks about code readability as a positive, but nobody actually agrees about what it means. The problem is readability depends on the person reading.

Your code use a particular approach, which is mapping over an Optional value, in order to execute some code only if the optional value is present. This approach would be very familiar to anybody coming from say Haskell where this is a common idiom. In Java this idiom is less common though because Optional is only a recent addition to the language, and there are already established other idioms for such things. So for many Java developers, the intention of the code will not be immediately obvious, and therefore readability will suffer. But it really depends on the audience for the code, and what idioms and conventions are established for the code base in question.

Personally I find your example less readable than a conventional if because it breaks some conventions I would expect. For example that stream operations are functional and doesn't have side effects.

JacquesB
  • 57,310
  • 21
  • 127
  • 176
  • 1
    Especially agree with last paragraph. That "map" should be a "filter", followed by a forEach or similar. (I'm a bit stale on the Java syntax) – user949300 Jan 20 '18 at 21:08
  • Yep. It's important to learn the idioms used by your team before churning code out. – svidgen Jan 20 '18 at 21:12
4

It harms readability because it requires more mental “scratch space” for the programmer. For procedural programs, each statement can be considered in relative isolation. Some operation works on some input, maybe has some side effects, and maybe changes some state. With chained operations, the person reading the code needs to mentally accumulate all of the operations and side effects until the end of the statement lest something clever is placed at the end that doesn’t execute the series of operations immediately.

But that is a relatively minor complaint.

The bigger problem with this manner of programming is that it is a pain and a half to debug since most tools make it difficult to see the intermediate steps of the statement.

Telastyn
  • 108,850
  • 29
  • 239
  • 365
  • 3
    I agree completely with your last sentence. Invariably I break these things into separate variable assignments until they're properly debugged, and often I'll just leave them that way for production. – Robert Harvey Jan 20 '18 at 18:17
  • @RobertHarvey, I also agree that the main problem with these programming styles has less to do with inherent readability (though some may initially find it difficult to read simply through being unaccustomed) and more to do with deficient debugging tools and/or inadequate language syntax. Assignment to implicitly-typed intermediate variables (esp. terse ones like `step1`, `step2`...) is the easy solution to that. – Steve Jan 20 '18 at 19:45
1

What you refer to is known as "point-free" style. Its readability is not universally considered bad.

Advantages of point-free style :

  • It makes the structure of the computation more readable.
  • Less variable names => less possibility to make mistake.

Readability of point-free style :

Depending on the case :

My rule of thumb for beginners : if the style gets rid of a temporary variable that would be named without sense [like "x", "tmp", "temp", "result"] then better use the point-free style. Otherwise as you and your colleagues like.

For myself : the shorter version is usually more readable.

Depending on the person :

The point-free style usually takes some getting used to. Some people [many from the imperative programming world] dislike it. But the getting used to comes very quickly with practice.

libeako
  • 159
  • 3
1

Also take into account the background of the people doing your code reviews. If, for example, they're experienced software engineers who have been working with C/C++ for most of their lives (say, 20-30 years), anything that's not imperative and state-mutating might seem strange and unnecessary for them as readability depends on what the person is used to be seeing. Think about all those cases where spaghetti code in old mature projects looked horrible to you but was "just fine" for the veterans. It's easy to become biased.

Readability imho. is about everything but experience - readable code should be intuitively understood without much effort.

Petras Purlys
  • 368
  • 1
  • 9