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

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 2

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 3

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 4

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 5

https://computing.llnl.gov/linux/slurm/coding_style.pdf

Answer 6

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