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

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 2

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 3

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 4

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 5

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;
}