Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have read some code and here are two ways to check and log the error in cpp code that repeats in my project many times. Which way is better?

1.

bool AClass::myMethod()
{
     if (!SomeCondition())
     {
         Warning("AClass::myMethod: your description");
         return false;
     }
    doSomthing();
    return true;
}

2.

bool AClass::myMethod()
{
    if (SomeCondition())
    {
        doSomething();
    }
    else
    {
        Warning("AClass::myMethod: your description");
        return false;
    }
    return true;
}
share|improve this question
    
Why not separate validation from the action itself? –  Neil May 2 '12 at 9:23
4  
If you are checking pre-conditions of the input parameters to the method then it may be worth using (or thinking about) exceptions rather than returning false. –  Loki Astari May 2 '12 at 19:35

7 Answers 7

up vote 20 down vote accepted

I think that in this specific case, both solutions are equally well readable.

However, if there are other checks added later to verify that the function can continue to process, then only the first solution will be easily extended without making the code difficult to follow:

bool AClass::myMethod()
{
     if (!SomeCondition())
     {
         Warning("AClass::myMethod: your description");
         return false;
     }

     if (!SomeCondition2())
     {
         Warning("AClass::myMethod: another description");
         return false;
     }

    doSomthing();
    return true;
}

is clearer than

bool AClass::myMethod()
{
    if (SomeCondition())
    {
        if(SomeOtherCondition())
        {
            doSomething();
        }
        else
        {
            Warning("AClass::myMethod: another description");
            return false;
        }
    }
    else
    {
        Warning("AClass::myMethod: your description");
        return false;
    }
    return true;
}

So I would personally use the first version as soon as more than one check is necessary.

share|improve this answer
1  
Also known as "guard statements", I strongly prefer them over deep nesting. +1 –  Izkata May 2 '12 at 20:38

I would go a step back and say why to limit yourself in those two cases? A cleaner and more defensive technique is suggested in the pragmatic defense. By following the article's suggestions you end up in a structure like that:

bool AClass::myMethod() throw()
{      
    bool myMethodReturn = false;

    try
    {
       // your preconditions here
       // on failure throw an exception

       // main code...

       // your postconditions here
       // on failure throw an exception

       // everything went fine
       myMethodReturn = true;
    }
    // catch all the possible exceptions
    catch (const std::domain_error& err)
    {
        // log the error
    }
    catch (...) 
    {
        // log the critical error
    }

    return myMethodReturn;
}
share|improve this answer

With such a short method, it doesn't matter much.

If the method would be long, then, well, one would better refactor it into shorter methods, up to the moment until, again, it wouldn't matter much which of your options to choose.

Actually, this could be a good criteria for when to stop refactoring: when different options begin looking "hard to choose from", "all roughly good enough", it's pretty safe to stop there.

I learned about that when studying Doug Lea's CPJ. While reading it, I noticed that in code snippets author uses a style I positively hate (else-after-return). I was planning to point this as a drawback in book review, but upon completion I discovered feeling uncomfortable about pointing finger at that. Since I usually prefer to be vocal about style issues, I thought what's wrong with me, got back and re-checked all the "bad" snippets. In that back-review, I noticed all the methods were pretty small (less than 10 lines or something), and particular style preference didn't matter much indeed.

share|improve this answer

If the purpose of this method is to effect a state change, then it should either make the state change or throw an exception. Log the problem when the exception is caught. Also, as much as possible, avoid writing classes that can be in a state where some method calls are invalid.

void AClass::myMethod(**args**) {
  if (!SomeCondition(**args**)) { throw new InvalidArguments(); }
  performAction(...);
}

try {
  AClass::myMethod();
}
catch (...) {
  Log(...);
}
share|improve this answer

I humbly offer this as an alternate to 1 and 2:

3.

bool AClass::myMethod()
{
     bool valid = true;
     if (!SomeCondition())
     {
         Warning("AClass::myMethod: your description");
         valid = false;
     }

     if(valid) 
     {
         doSomthing();
     }
     return valid;
}

This is perhaps the most verbose of the three, but I think it is both clear and efficient, since in theory you shouldn't return in the middle of a method since it's not as fast. Plus you can perform both specific and general failure code by performing actions in the initial validation phase and in the "else" of the if(valid) clause.

Better still, separate validation from the action by using two methods:

3.

bool AClass::isValid() 
{
     bool valid = true;
     if (!SomeCondition())
     {
         Warning("AClass::isValid: your description");
         valid = false;
     }    
     return valid;
}

bool AClass::myMethod()
{
     bool valid = isValid();
     if(valid) 
     {
         doSomthing();
     }
     return valid;
}
share|improve this answer
4  
"return in the middle of a function is not as fast" - ??? –  Martin Ba May 2 '12 at 10:58
    
@Martin, that's what I've heard, though not sure what's that's based on. Let me see if I can fetch an article which proves that. –  Neil May 2 '12 at 12:23
1  
@Martin, take a look at this question which addresses this. Apparently it's bad simply because it's not straight forward coding at least for what concerns Java, though it used to be based on performance. Not sure if that holds true also for C++. –  Neil May 2 '12 at 12:25
6  
if (Optimization.isPremature()) { return sqrt(All_Evil); } –  Secure May 2 '12 at 16:32
    
I could understand if it made the code clearer when not optimized, but not only is it clearer this way in my opinion, you don't risk to put code at the end of the method and not having it run. @Secure, And incidentally, the sqrt(All_Evil) factors out to be a value between i and money. –  Neil May 3 '12 at 10:22

Which way is better?

The better is what is more understandable.

In my opinion, the first version, where the condition is not negated is better. Negating a condition makes the function more complex, therefore it is bit harder for understanding.

share|improve this answer
    
The first one also reduces the indentation. –  ChrisMcB May 2 '12 at 9:08

In my opinion, the first alternative is the clearly best. The normal scenario is to return true, and get to the doSomething, while the error logging can be looked at as a deviation of the normal scenario. In my opinion, these should resemble an exception, in that it is thrown/logged as the first thing that happens in the function. The latter alternative looks a bit messy in my eyes.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.