Better way of handling pre conditions and logging

Question

1. Imagine you have the following:

void Foo::doFoo()
{
    if (!isConditionValid())
    {
        log("doFoo not possible because condition is not valid");
        return;
    }

    if (!isTheOtherConditionValid())
    {
        log("doFoo not possible because the other condition is not valid");
        return;
    }

    // do Foo actions
}

Is there a better way to format the code? Really often, I have 1,2 or 3 conditions, and I find it really repetitive to have this structure.

2. What if I have the doBar method, that has the exact same conditions, so we would have:

void Foo::doBar()
{
    if (!isConditionValid())
    {
        log("doBar not possible because condition is not valid");
        return;
    }

    if (!isTheOtherConditionValid())
    {
        log("doBar not possible because the other condition is not valid");
        return;
    }

    // do Bar actions
}

I feel like I should refactor those conditions, but I find it difficult to find the name of the method. If call the method "areConditionsValid", I don't expect it to log anything, just to return a boolean. So the following doesn't seem right to me:

bool Foo::areConditionsValid(String action)
{
    if (!isConditionValid())
    {
        log(action + " not possible because condition is not valid");
        return false;
    }

    if (!isTheOtherConditionValid())
    {
        log(action + " not possible because the other condition is not valid");
        return false;
    }

    return true;
}

Because it does more than what it says it does.

I can't call it checkConditionsAndLog() because it is not good practice to have a "And" in the method's name, it is a good way to see that it's doing more than it should do...

What's the best way to refactor this?

The same applies if there is only one condition to check.

PS: It's in C++

Answer 1

There is some ambiguity with the term "preconditions":

• it could mean "do not call the function if these are not valid" (that is, something that client code should handle)
• it could mean "if these are not valid in the function, (log message and) compute nothing"
• it could mean "if these are not valid exit the application"

When implementing this, you should rarely decide on how to handle the error, at the place where it occurs; instead, you should throw an exception and let client code decide.

With this, your code could become:

void Foo::AssertPrecondition(bool condition, std::string msg)
{
    if(!condition)
        throw std::runtime_error{ std::move(msg) };
}

void Foo::doFoo()
{
    AssertPrecondition(isConditionValid(),
        "doFoo not possible because condition is not valid");
    AssertPrecondition(isTheOtherConditionValid(),
        "doFoo not possible because the other condition is not valid");

    // TODO: implement valid execution branch
}

From this point onwards, client code can decide what happens when an error occurs.

Also, as a rule, if you have more than one repeating line in code, you should refactor those to a separate function.

Answer 2

A problem I can see with your approach is that your preconditions return, instead of throwing an exception. Usually, preconditions throw an exception when violated. Since you are using C++, asserts can be used instead.

If you use exceptions/asserts, an additional refactoring can be made easily. Instead of:

if (!isConditionValid())
{
    log("doFoo not possible because condition is not valid");
    assert(false);
}

if (!isTheOtherConditionValid())
{
    log("doFoo not possible because the other condition is not valid");
    assert(false);
}

// do Foo actions

you can have a method such as:

void Contracts::require(predicate, message)
{
    if (!predicate())
    {
        log(message);
        assert(false);
    }
}

and you call it this way:

Contracts::require(isConditionValid, "doFoo not possible because condition is not valid");
Contracts::require(isTheOtherConditionValid, "doFoo not possible because...");

// do Foo actions

Answer 3

Simply use an implementation of code contracts, in some implementations such as one provided in. Net these conditions can be checked in compile time.

https://github.com/Microsoft/CodeContracts

Answer 4

You can also calculate the result, and log if it fails.

Something like this :

bool Foo::areConditionsValid(String action)
{
    const bool result = isConditionValid() &&
                        isTheOtherConditionValid();

    if (!result)
    {
        log(action + " not possible because some conditions are not valid");
    }

    return result;
}

I also find this more readable :

void Foo::doBar()
{
    if (!isConditionValid())
    {
        log("doBar not possible because condition is not valid");
    }
    else if (!isTheOtherConditionValid())
    {
        log("doBar not possible because the other condition is not valid");
    }
    else
    {
        // do Bar actions
    }
}