Coding Standards / Coding Best practices in C++

Question

Consider the two code segments below. Which one is better and Why? If you have any other idea, please do mention. Where can I find answers to coding practices like these? If you are aware of any book / article, please do share.

//Code 1

bool MyApplication::ReportGenerator::GenerateReport(){
    bool retval = false;
    do{
        if (!isAdmin()){
            break;
        }
        if (!isConditionOne()){
            break;
        }
        if (!isConditionTwo()){
            break;
        }
        if (!isConditionThree()){
            break;
        }

        retval = generateReport();
    } while(0);
    return retval;
}

//Code2

bool MyApplication::ReportGenerator::GenerateReport(){
    if (!isAdmin()  || !isConditionOne() || !isConditionTwo() || !isConditionThree()){
        return false;
    }
    return generateReport();
}

The clean code by Robert C. Martin is a nice book which deals with this. But, I guess that book is inclined towards Java.

UPDATE:

1. I have purposefully used do{ }while(0); loop as I did not wanted to use goto.

2. I wanted to get rid of so many if and break statements, so I proposed Code 2.

What I can see from the replies is a mixed set of responses for both the Code1 and Code2. There are people who prefer goto as well compared to Code 1 (which I thought was better).

Answer 1

It really depends on the future expectations of the code. Code1 above implies that there may be additional logic to be added for each of the conditions; Code2 above implies rather that there is a rational grouping of the conditionals. Code1 may be more relevant if you expect to add logic for the conditions at a later date; if you don't, though, Code2 is probably more sensible because of the brevity and implied grouping.

Answer 2

I don't really like using a do/while loop in this way. One other way to do this would be to break your conditional in Code2 into separate if checks. These are sometimes referred to as "guard clauses."

bool MyApplication::ReportGenerator::GenerateReport()
{
    if (!isAdmin())
        return false;

    if (!isConditionOne())
        return false;

    // etc.

    return generateReport();

}

Answer 3

Personally I prefer sample 2. It groups those items that will not result in the report being generated together.

As far as general coding guidelines, the book Code Complete (Amazon) is a good reference for coding style issues.

Answer 4

Which on do you think best expresses what the code is trying to say. Which one do you need to work hardest to understand?

I would do this:

bool MyApplication::ReportGenerator::GenerateReport(){
    if (isAdmin()  && isConditionOne() && isConditionTwo() && isConditionThree()){
         return generateReport();
    } else {
        return false;
    }

}

Because:

a). Prefer to say what I want rather than what I don't want b). prefer symmetry, if and else. Clearly all cases covered.

Answer 5

bool MyApplication::ReportGenerator::GenerateReport(){
    if ( ! isAdmin         () ) return false ;
    if ( ! isConditionOne  () ) return false ;
    if ( ! isConditionTwo  () ) return false ;
    if ( ! isConditionThree() ) return false ;
    return generateReport() ;
}

Answer 6

I've used similar to both in different circumstances but you've overcomplicated the first IMO:

bool MyApplication::ReportGenerator::GenerateReport(){
    bool retval = false;
    if (!isAdmin()){
    }
    else if (!isConditionOne()){
    }
    else if (!isConditionTwo()){
    }
    else if (!isConditionThree()){
    }
    else
        retval = generateReport();
    return retval;
}

Answer 7

Regarding example 1: If you want goto, why don't you write goto??? It's much clearer than your suggestion. And considering that goto should only be used rarely, I vote for #2.

Answer 8

I would personally prefer a variation on your second code segment. The short circuiting will work the same, but the conditional is less verbose.

bool MyApplication::ReportGenerator::GenerateReport()
{
    if(isAdmin() && isConditionOne() && isConditionTwo() && isConditionThree())
    {
        return generateReport();
    }

    return false;
}

It says everything in one nice clean spot. "If all these conditions are met, then do this. Otherwise, don't."

I feel like your first code segment makes that logic harder to see by spreading the condition across 12 lines. Also, the encasing loop might cause someone to do a double take.

Answer 9

Code 1 is, IMO, worst as it does not immediately convey the intendend meaning which is to generate the repoort only in certain circumstances.

Using:

   if (condition_1) return false;
   if (condition_2) return false;
   ...

would be better.

Also, what I dislike in code 1 is the fact that it try to mask gotos using a while and breaks (which are gotos). I would then prefer to use directly a goto, at least it would have been easier to see where the landing point is.

Code 2 might be formatted to look nicely, I guess:

 bool MyApplication::ReportGenerator::GenerateReport(){
     if (!isAdmin()        || !isConditionOne() ||
         !isConditionTwo() || !isConditionThree()) {
        return false;
     }
     return generateReport();
 }

Or something similar.

Answer 10

I prefer a modification of sample 2:

bool MyApplication::ReportGenerator::GenerateReport()
{
    bool returnValue = false;
    if (isAdmin() &&
        isConditionOne() &&
        isConditionTwo() &&
        isConditionThree())
    { 
        returnValue = generateReport();
    } 
    return returnValue;
}

It has the benefit of having a single exit point for the function, which is recommended for easier maintenance. I find stacking conditions vertically instead of horizontally easier to read quickly, and it's a lot easier to comment individual conditions if you need to.

Answer 11

A switch would be a better solution to your problem I think. You would need to overload the method to take in an int param and I don't know if that's something you would want to do or not.

Bool MyApplication::ReportGenerator::GenerateReport(int i)
{
  switch(i)
  {
    case 1:
       // do something
       break;
    case 2:
       // do something
       break;
   // etc
 }
 return GeneratReport()
}

Not really sure what your plan is since you're calling the method recursively and as some point you will want to leave the method.

Answer 12

bool MyApplication::ReportGenerator::GenerateReport()
{
   return isAdmin()  
      && isConditionOne() 
      && isConditionTwo()
      && isConditionThree()
      && generateReport();    // Everything's ok.
}

Answer 13

https://computing.llnl.gov/linux/slurm/coding%5Fstyle.pdf

Answer 14

Code Complete is a commonly recommended book that goes into some detail about those kinds of stylistic issues. Also consider taking a look at some of the published organization style guides to see if they have any opinions on the issue; Google's Style Guide, for instance.

As to what I prefer, the second example is much better to my mind. The first is essentially an abuse of the do {} while construct to avoid using a goto, dogmatically sticking to the letter of "avoid gotos at all cost" while missing its spirit of "code for clarity, don't use unobvious language tricks".

In fact, the only reason to even use a goto at all would be to dogmatically stick to a "only one return statement per function" approach when you could get away with a simple, readable

if (!isAdmin()){
    return false;
}
else if (!isConditionOne()){
    return false;    }
else if (!isConditionTwo()){
    return false;    }
else if (!isConditionThree()){
    return false;    }
else
    return generateReport();

Other thoughts?

Don't name local variables that are used to hold a computed success state "returnValue" or similar. Obviously whatever you return is the return value, any one who can read C can see what is being returned. Tell me what computation it holds. The name "returnValue" gives me no information as to what it means when it is true or false. In this example "couldGenerateReports" or similar would be far more preferable.

Answer 15

Personally I prefer my for, while and do ... while loops to be actual loops. In the first code example this is not the case. So I would opt for example 2. Or, as others have already said, for breaking example 2 into a number of if ... return statements.

Answer 16

I like the answers that are a variation of version 2, but just to give an alternative: If those conditions are logically tied together, chances are that you will need to check for them again in other places. If this is true, then maybe a helper function would do the job better. Something like this:

bool isReportable(anyParametersNeeded){
    //stuffYouWantToCheck
}

bool MyApplication::ReportGenerator::GenerateReport(){
    if (isReportable(anyParametersNeeded)){
        return generateReport();
    }
    return false;
}

As this function is small, maybe you can even inline it (or let the compiler decide ;)). Another bonus is that if you want to include some extra checks in the future, you only have to change that function and not every spot where it's used.