Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);

    fillImages(data);
    // 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.

PS: I hope this is an appropriate question for Code Review.

share|improve this question
add comment

6 Answers

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.

share|improve this answer
    
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
1  
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 –  Joset 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
add comment

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 :)

share|improve this answer
    
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
add comment

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.

share|improve this answer
    
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
add comment

You can always format v1 like this for more readability:

    return insertToDb(
            fillImages(
                    fetchInfo(REQUEST_URL, amount))
    );
share|improve this answer
2  
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
add comment

My answer is:

protected Boolean doSomething(int amount) {
    return 
       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.

share|improve this answer
3  
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
add comment

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

share|improve this answer
    
Hey, thanks. I didn't know of that. –  zidarsk8 Jan 19 '13 at 1:36
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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