if else coding style

[Peter Kovac]

Hi I was wondering which is a preferred way of writing methods like this:

int meth ()
{
     final int Y = ....
     int result = ...

     if  ( result < 0 )
     {
         // some calculation
         int calRes = ...
         return calRes;
     }
     else
     {
         return Y;
     }
}

The code above uses 'else' but I think the following is better:

     if  ( result < 0 )
     {
         // some calculation
         int calRes = ...
         return calRes;
     }

     return Y;

Which is the recommended?

[fred rosenberger]

Some folks would say neither. a LOT of people think there should only be one exit point from a method, so they'd recommend this:

final int Y = ....  
if  ( result < 0 )  
     {  
         
         Y = // some calculation  
     }  
  
return Y;  

update...i'm not sure this will work with Y being declared 'final', but the point is still valid...

[Santiago Tovar]

I prefer less code lines... but the bettter way is the one posted by fred rosenberger

[Anayonkar Shivalkar]

Well, I would prefer below:

    int meth ()  
    {  
         final int Y = ....  
         int result = ...  
         int valueToReturn = 0;
      
         if  ( result < 0 )  
         {  
             // some calculation  
             int calRes = ...  
             valueToReturn = calRes;
         }  
         else  
         {  
             valueToReturn = Y;  
         }
         return valueToReturn;  
    }  

i.e. only one exit point and return value can be clearly deduced from conditions.

[Santiago Tovar]

fred rosenberger wrote:Some folks would say neither. a LOT of people think there should only be one exit point from a method, so they'd recommend this:

final int Y = ....  
if  ( result < 0 )  
     {  
         
         Y = // some calculation  
     }  
  
return Y;  

update...i'm not sure this will work with Y being declared 'final', but the point is still valid...

It does not work because Y is final.

[Steve Luke]

Santiago Tovar wrote:

fred rosenberger wrote:...
update...i'm not sure this will work with Y being declared 'final', but the point is still valid...

It does not work because Y is final.

Yeah, that was a mistake. If we assume that Y should be final and should be defined inside the method then fred's code should be written something like:

final int Y = ....
int returnValue = Y; //or a better name...  
if  ( result < 0 )  
     {  
         
         returnValue = // some calculation  
     }  
  
return returnValue;  

[Junilu Lacar]

Standard consultant's reply: It depends.

1. If you don't have a team coding standard, then no foul no harm. Maybe talk to your teammates about adopting one for future reference.
2. If you have a team coding standard, try to follow it.
3. However, if you think that an exception to the coding standard can be justified, discuss it with someone on your team.
4. If your teammate(s) agree with the break from the standard, go with it. Consider updating the standard to call out the exception.
5. If they don't agree, ask yourself if it really hurts you to follow the standard. If it doesn't, go with the standard.
6. If you and your teammates just can't come to an agreement about the break from the standard, your problem is more than just a coding standard.

I have commented on this in a previous post but I'll repeat here since I'm too lazy to find the link. The "one exit point" rule is a good rule of thumb to follow. It's especially good to follow in code that is more than 20 lines long. However, a justifiable and well-placed return in a short method does not do much harm. See the refactored code in this Composed Method example. Here, the guard/sentinel clause idiom calls for an immediate return. The other exit point is the end of the method, where execution just "falls out" but having the two exit points doesn't seem at all bad.

Another case in point:

@Override
public boolean equals(Object o) {
    if (o == this) 
        return true;
         
    if (!(o instanceof MyType))
        return false;

    ...
}

This is part of a typical implementation of Joshua Bloch's recipe for overriding the equals method. It is clear from the first two if-statements that no other calculations need to be done if any of these conditions are true, so just return the result immediately. This clearly violates the "one exit point rule" but it doesn't really matter that much. Why? Because the rule is there for a reason which is mainly to improve code clarity. When you have unclear and convoluted code, multiple exit points just add to the confusion and can create more problems. However, if you effectively attack and resolve any problems having to do with code clarity, you might find that having multiple exit points is actually a non-issue. That's exactly the case in the equals() recipe implementation above. Any attempts to force conformance with the "one exit point" rule might actually result in more complex code if you end up with a deeply nested if structure.

[Peter Kovac]

Thanks Junilu.