The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

Thoughts on Java code readability

Hi, all.

I've been working closely with a Java developer whose skills vastly dwarf mine.  It's kind of refreshing, honestly, having someone pick apart a lot of what I've been writing; in my previous job, I was the only person to work on my particular application.  I never had anyone to turn to for critique (friendly or otherwise).

In any case, he was looking over my shoulder as a typed out a fairly innocuous stub, and I actually heard him gasp.  This is what I typed:

if (failureCount > 2)
{
  dispatchFailureAction("You have failed.");
  return ("accountresetaction.transfer_call");
}
else
{
  dispatchTryAgain("Try once more.");
  return ("accountresetaction.prompt_for_auth");
}

He informed me that this style of coding is frowned upon, and it's much easier to read code that's written in this fashion:

if (failureCount > 2)
{
  dispatchFailureAction("You have failed.");
  return ("accountresetaction.transfer_call");
}

dispatchTryAgain("Try once more.");
return ("accountresetaction.prompt_for_auth");

So, is there a general consensus on these kinds of things?  I've been hammering out the "else" statement with glee, since about 1995, and have never once been told that my code looked unpleasant.  Have I been constructing ugly syntax, unknowingly?

Thanks,
Andy
Andy Nonymous Send private email
Monday, July 23, 2007
 
 
No, Andy, I like your style ok.  It would probably be easier to maintain.

The reason some people (including myself) might leave the 'else' clause out, and use the 'return' to prevent fall-through, would be if there's a LOT of 'if' clauses, which would result in a LOT of 'else' clauses, and that 'tree of close brackets' at the end of your routine.

When I DO code without the 'else', I tend to put in comments like "// When here, you know that xyz is true, abc hasn't happened, and you're about to check xyz2"
AllanL5
Monday, July 23, 2007
 
 
You're style is fine, Andy.

What _is_ relatively unreadable, is 3 or 4 levels of 'if' inside 'if'. They can usually replaced with early exits and removing elses, which is what your colleague has done.

But the simple case you described does not warrant a change.
Ori Berger Send private email
Monday, July 23, 2007
 
 
As far as i am concerned i like code like follows (if it is not too complicated to do so). So you have only one possibility to exit the codeblock, which is easy to follow/debug. I only put returns either straight on to the top of the method to check conditions or at the bottom as the only method exit.

String ret = null;

if (failurecount>2)
{
  dosmth();
  ret = "failure";
} else
{
  dosmthelse();
  ret= "tryagain";
}

return ret;
Nils Drews Send private email
Monday, July 23, 2007
 
 
I personally have long written

if (condition)
  return a
else
  return b

but my IDE and bug finder highlight it as an unnecessary else so I've been taking them out lately. As long as methods are short enough to comprehend, nits like this don't matter too much.
Stan James Send private email
Monday, July 23, 2007
 
 
Folks try to save space/code/indentation, and once things start that way, they tend to stay that way, becoming a tradition. :-)

I like your own version, though I can imagine many folks preferring the other style. For example, folks who might use JavaScript or who develop open source software.
Joao Pedrosa
Monday, July 23, 2007
 
 
BTW, in Ruby, the return command is optional, because the last statement can be used as the returned parameter. For example, here's a piece of code on which I have been currently working:

    def ultimate_content
      if @template_window
        @template_window.ultimate_content || content || render
      else
        content || render
      end
    end
Joao Pedrosa
Monday, July 23, 2007
 
 
Wow, the old "let's pretend that a failure count is meaningful" anti-pattern. Yes, I've done that, and always felt completely ashamed by injecting such non-determinism into my own code. But, expedience rules as always.

Oh, wait, you were talking about whether to use an unnecessary "else" block?

Nevermind.
my name is here
Monday, July 23, 2007
 
 
Andy, your style is OK. It's not a big thing.

However I do prefer your colleague's style. The extra 'else' makes it more noticeable to someone who is scanning the code quickly (and doesn't notice the return) that these are alternatives. It also protects a bit against someone modifying the 'if' structure in some way. The difference certainly isn't enough for me to say 'your code is bad' and it's a bit of a personal preference thing.

If it's house style to do it your colleague's way then you should get into the habit of doing it the same way everyone else does. Consistent style helps the readability of code a lot, even if that style isn't the absolute optimum best style.
DJ Clayworth
Monday, July 23, 2007
 
 
"In any case, he was looking over my shoulder as a typed out a fairly innocuous stub, and I actually heard him gasp."

Really a gasp?  Some people get too attached to their way of doing things and conflate it into it being the way that things must be.  I have no trouble with your code or his.

I have my own ways of coding and indenting.  If the standard is different at some POE, I use that standard unless it is obviously broken.  Disagreement with the standard is not enough to refuse to use the standard.

Sincerely,

Gene Wirchenko
Gene Wirchenko Send private email
Monday, July 23, 2007
 
 
It's completely a personal preference.  Half of us find the way you wrote it to be more maintainable, and the other half thinks the first half is a bunch of morons.  It's one of those things that we burn into our heads as we write our own code, so anything that doesn't match that burn appears too foreign, and we have to stop to actually read it.

My personal preference is with the guy who gasped, but I wouldn't feel so strongly about it as to be audible :)
JohnB
Monday, July 23, 2007
 
 
"..that 'tree of close brackets' at the end of your routine..."

So JAVA does not have 'else if' thing?
Like in C++, for example?
asmguru62 Send private email
Monday, July 23, 2007
 
 
Both ways are fine. He overreacted.
cbmc64 Send private email
Monday, July 23, 2007
 
 
It depends.

If the conditions are symmetric, I'd use your style:

if (x % 2 == 0) {
    return "even";
} else {
    return "odd";
}

(Although in this contrived example, I'd probably use the ternary operator.)

Your colleague's style is called "guard clauses."  I use it where I'm basically trying to get rid of error cases before my actual code:

if (error1) {
    return "error1";
}

// normal case follows here, not nested
if (error2) {
    return "error2";
}

// normal case follows here, not nested
if (error 3) {
    return "error3";
}

// normal case follows here, not nested
do_normal_processing();
Michael G Send private email
Monday, July 23, 2007
 
 
I'd be more inclined to write it your colleague's way, without the else, but it's not that important in this case.

When the if statement has multiple cases, I prefer guard clauses with returns.  I guess that besides feeling cleaner, it also tells me that no one will come along and make weird things happen further down the function, because I've already returned out of it.
Kyralessa Send private email
Monday, July 23, 2007
 
 
Probably his style has a better McCabe metric, but in this case it's unimportant.

The McCabe metric looks at nesting levels. Too many nesting levels render code unreadable and unmaintainable. However code like:

if( condition a) {
 ...
} else if (condition b) {
 ...
} else if (condition c) {
 ...
} else {
  ...
}

is quite readable and fairly maintainable IMO although its McCabe metric is not that great.

Here's a different example.
Resource r = null;
try {
  r = openResource();
} catch (Exception e) {
  handleException(e);
} finally {
  try {
      if(r != null) closeResource();
  } catch (Exception e) {
      handleException(e);
  }
}

could be rewritten as:
Resource r = null;
try {
  try {
    r = openResource();
  } finally {
    if(r != null) closeResource();
  }
} catch (Exception e) {
  handleException(e);
}

Second form has a better McCabe metric, however it fails to capture the exact exception scope.
Dino Send private email
Monday, July 23, 2007
 
 
Dino, lol at McCabe and the stuff we had to go through in the past to keep it under 10 per function. Good memories. I would do stuff like avoid else clauses and let the return fall through because the else cost a McCabe and we had to use them very carefully.

It wasn't necessarily better code, it was just "good" from the strict standpoint of the McCabe complexity metric, which was vitally important on that project.
cbmc64 Send private email
Tuesday, July 24, 2007
 
 
@cbmc64

Agreed. I use McCabe metric to just highlight potentially bad code. But I found the metric to give lots of false alarms.

Secondly, I find that hard rules don't work in general. People act irresponsibly when applying hard rules - usually blindly: "It's the rule, it's not me".

I prefer guidelines to rules because they require people to act responsibly and think before doing.
Dino Send private email
Tuesday, July 24, 2007
 
 
I'd have to agree with Ori. In the long run, guard clauses and similar padding provide more benefit than clear, concise, minimal, "do the job and nothing more" code.

Those benefits are neglible in simple examples like the ones posted.

They however are apparent when dealing with fairly complex structures and in environments when you may have many programmers working with the same code, either as part of a joint effort or individually as the software moves through the maintenance lifecycle.

I wouldn't rule out the mentor's approach entirely because on relatively small projects done by one or two developers, there's no point in subjecting yourself to potential bugs involving misplaced brackets and unpaired quotes. Out of sequence or out of loop logic errors are much easier to catch.

Guard clauses really prevent other developers down the road from inserting code and not realizing they're creating looping logic errors further down the screen, hence their benefit on large teams.
TheDavid
Wednesday, July 25, 2007
 
 
I always leave out the else when it's not necessary, but only *because* it's not necessary.  I don't think it hurts readability to leave it in.
Matt Brown
Thursday, July 26, 2007
 
 
I agree with both Andy and Nils :)

If a code block is really a function, that is its job is to return some value, then I prefer the
if (something)
{
  return value1;
}
else
{
  return value2;
}

whereas if it a code block that is supposed to perform some process then I prefer the single exit point. In each case your code stresses the tyoe of thing being done (whether the important thing is the return value or the process being undertaken). YMWACV.

BTW, nice to see some people still prefer the only sensible brace style :)

Friday, July 27, 2007
 
 
> In any case, he was looking over my shoulder as a typed out a fairly innocuous stub, and I actually heard him gasp.

Someone who looks over my shoulder while I'm coding and gasps is liable to get stabbed in the face.

(I prefer his style, but it's just that, style.  Both styles are equally valid.)
Michael B
Tuesday, July 31, 2007
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz