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

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

Answer 2

My habit is to avoid if-blocks this way:

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

Answer 3

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.

Answer 4

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 5

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 6

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.