Which style is better to check and log the error?

Question

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

Answer 1

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.

Answer 2

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

Answer 3

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.

Answer 4

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

Answer 5

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(...);
}

Answer 6

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.

Answer 7

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