Explore our sites

Stack Exchange
tour help
Take the tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

Is it preferred to do:

if(a) {
    if(b) {
        //some statement(s)
    }
} else if(c) {
    //some statement(s) (the same ones as in the other block)
}

or

if((a && b) || (!a && c)) {
    //some statement(s) 
}

I currently have the first written. It feels easier to read and maintain, but duplicating the block feels like a smell to me. Is there a better third option that I'm missing?

share|improve this question
5  
You can save the two conditions to variables, and then simplify your if statement. var aAndB = a && b; var notAandC = !a && c; if (aAndB || notAandC). This reduces the number of text in the if statement, and allows you to use a nice variable name to describe the two conditions you are accounting for. –  willmel yesterday
 
@willmel Hmm. I like that. But I'm not sure I can find a good enough name to warrant a variable for it. –  Cruncher yesterday
1  
@Cruncher Then how do you warrant the if-condition itself? –  Simon André Forsberg yesterday
 
@SimonAndréForsberg I was thinking about that. Right now, I warrant it by the fact that it works. I'm afraid that I couldn't think of a natural name for !a && c that would be obvious for its existence. –  Cruncher yesterday
1  
How about if (a ? b : c) { ... }? –  matts yesterday
show 2 more comments

put on hold as off-topic by Jamal♦ yesterday

  • This question does not appear to be a code review request within the scope defined in the help center.
If this question can be reworded to fit the rules in the help center, please edit the question.

2 Answers

up vote 10 down vote accepted

There are two ways to get around this type of situation.

Put some statements into a method:

void someStatements() {
 // statements
}

//...
if(a) {
    if(b) {
        someStatements();
    }
} else if(c) {
    someStatements();
}

Put the evaluation in a method:

boolean shouldDoStatements() {
  return (a && b) || (!a && c);
}

//...
if(shouldDoStatements()) {
    //some statement(s) 
}

My preference would be for the latter. It keeps the main branch of code clean instead of nested ifs. Also, If you have to do any computations to produce a, b or c, you don't have to worry about polluting the scope with extra variables.

share|improve this answer
1  
In my actual code right now, it's actually a single statement block. But I do like your latter solution. P.S Was the double indent intended in the else if block? –  Cruncher yesterday
 
@Cruncher: The double indent was just a copy and past error I didn't catch. Fixed now. –  unholysampler yesterday
 
I wasn't sure. That could have genuinely been by design to match the level of the other statement –  Cruncher yesterday
add comment
up vote 3 down vote

@unholysampler's answer is excellent and is how I would do it.

To go into a bit more detail on the why of things though - there are a number of reason to avoid repeated code.

  1. It's not always obvious at a glance whether the code is actually identical or not, sometimes there can be subtle differences.

  2. This can make the code generally harder to understand as it tends to "bulk out" the methods etc.

  3. Sometimes you find and fix a bug in one section of the code and forget to do it in the other.

  4. The code will use more space and run slower (as it will be optimised less often, be less likely to all be in the cache, etc).

Basically duplicated code = more time to write, more time to maintain, more hidden and subtle bugs. If you ever see duplicated code in your software you should always at least look to see if it can be removed (sometimes it can't, but 99% of the time it can).

share|improve this answer
add comment

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