Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I wonder what is better code practice or just what looks better in Java:

Version 1:

protected Boolean doSomething(int amount) {
    if (amount < 1) return false;

    return insertToDb(fillImages(fetchInfo(REQUEST_URL, amount)));

Version 2:

protected Boolean doSomething(int amount) {
    if (amount < 1) return false;

    Data[] data = fetchInfo(REQUEST_URL, amount);

    // or even
    // data = fillImages(data);

    return insertToDb(data);

This could be a problem if we had more functions chained like that. I would like to make my code nicer, for anyone else who might have to edit it, and just to know that I am not writing anything that looks stupid.


locked by Jamal Dec 21 '14 at 22:01

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site, so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. More info: help center.

closed as off-topic by Jamal Oct 5 '14 at 22:54

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

up vote 13 down vote accepted

I definitely prefer the second version. The steps taken just "leap out" of the code and the method is easier to read. It will also be a lot easier to debug (or to pinpoint the problem in a stack trace).

Always go for readability, it makes for easier understanding when going back to your old code and it's a lot easier for someone else to understand what is going on.

I also prefer the "data = fillImages(data)" if possible. It makes clear that the data is modified.

Than you for the input. Still the data = something(data) style looks really funny to me, but I guess it does make it quite obvious that something has been changed, and I see how this is useful for later times. – zidarsk8 Aug 7 '11 at 0:02
Version 2 also improves merging efforts with SCM. – Jeremy Heiler Aug 7 '11 at 15:07
@Jeremy: Had not really thought of that but that definitely would make things easier. – Eric-Karl Aug 7 '11 at 15:48
IMO, we don't really want methods modifying outside-referenced values in Java – eradicus Aug 8 '11 at 2:41
Another reason for Version 2 is also that it forces the relavant classes to be declared as an import, which is sometimes used to calculate the complexity of a class (the more classes it imports the more complex it is). – Dale Wijnand Aug 8 '11 at 16:56

In some cases method chaining improves readability (see this article) but you usually pay close attention to build your interface to be fluent and most APIs are not designed that way.

In your case, method chaining did not improve readability, so I prefer the second version.

One of the challenges to write readable code is the fact that when you write it, you have all the context in your head. Most code that you write will make sense the time you write it. The challenge is to write code that you (and others) can understand fast when you do not have the context.

One of the things that helped me in the past is to revisit code I wrote months ago and try to understand it. If I struggled with understanding my own code, I tried to improve it to 'tell the story' better. Changed variable names, rearranged methods, extracted code into methods with good names, so the code read like a DSL, like a story.

Oops I guess this is not only about method chaining any more, though I hope it helps :)

Thank you for your thoughts and the article. I hope this way I can avoid future headaches while debugging :) – zidarsk8 Aug 7 '11 at 0:00

For me, the 2nd version is better, especially in this case:

protected Boolean doSomething(int amount) {
  if (amount < 1) return false;

  return insertToDb(fillImages(fetchInfo(REQUEST_URL, amount)), other_method_call());

as you hardly know the order of evaluation of the method calls (maybe Java defines something on this, but C does not). It could be:

  • fetchInfo, fillImages, other_method_call
  • fetchInfo, other_method_call, fillImages
  • other_method_call, fetchInfo, fillImages

See this "Parameter evaluation order before a function calling in C" for more information on this matter on the C language.

Thank you very much for this. It's always fun to learn new stuff like this, and it is certainly interesting to see how funny such undefined behavior is. Now I have to find out if that is also the case with Java :) – zidarsk8 Aug 7 '11 at 14:15

You can always format v1 like this for more readability:

    return insertToDb(
                    fetchInfo(REQUEST_URL, amount))
If you take the time to reformat, I strongly suggest that you split it in separate statements (like v2). The minute someone will use an auto-formatter in that code, that careful formatting will be over. And even without auto-formatting, someone else might very well not understand why it is formatted in such a weird way and "correct" it. – Eric-Karl Aug 7 '11 at 15:52

My answer is:

protected Boolean doSomething(int amount) {
       amount < 1 ?
         false : 
         insertToDb(fillImages(fetchInfo(REQUEST_URL, amount)));

If you plan to chain more functions just use whitespace. People have autoformatters so they can read it more easily, so they will change it (unless you provide autoformatter settings with your code).

Keep it short. Every new line you introduce is a place for another bug. Creating temporary variables is sometimes a mess because of naming issues which can lead to creepy bugs.

I fail to see how this is short. There is the same number of instructions as v2. However, this is obscure and convoluted, difficult to read and to debug. Every new instruction is a place for another bug, you do not have any less. – Eric-Karl Aug 8 '11 at 16:46

Neither. Both violate the Law of Demeter. Your example shows that you need to rethink the structure of your classes, and refactor.

Hey, thanks. I didn't know of that. – zidarsk8 Jan 19 '13 at 1:36

Not the answer you're looking for? Browse other questions tagged or ask your own question.