1

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++

  • I use isConditionValid() and throwIfConditionInvalid() respectively. (I use exceptions in projects whenever exceptions are allowed.) – rwong Aug 4 '15 at 8:44
  • Having the word "And" in the name is a smell, but it is not necessary to forbid it if the benefits outweigh the drawbacks. In your situation, it correctly describes what it does (and yes, it does several things) and it reduces code duplication (and therefore reduce the number of places where copy-and-paste errors can potentially happen). – rwong Aug 4 '15 at 8:48
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.

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
  • Forgot to mention it, but this is in C++. And from what I have seen, it's not such a good idea to extensively use the exceptions (stackoverflow.com/questions/4506369/…) – dyesdyes Aug 4 '15 at 9:11
  • @dyesdyes: I see. This being said, the article you link to suggests to use assertions instead, which is a perfectly viable alternative. – Arseni Mourzenko Aug 4 '15 at 9:15
  • 1
    Yes but asserts are only used in debug mode, I can't rely on them for production code. And also, if the condition is not met, I definitely don't want to stop the program, I just want to log then return, not more, not less. – dyesdyes Aug 4 '15 at 9:20
  • I see. Sorry, but since this is specific to C++, I can't help you more. It seems that you can still use exceptions, even if it's not the ideal solution. – Arseni Mourzenko Aug 4 '15 at 9:27
0

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

0

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
    }
}
  • You don't really answer the name issue I have. Formatting areConditionsValid this way only removes precision in the logs as you don't know which condition failed anymore. – dyesdyes Aug 4 '15 at 11:54
  • @dyesdyes I guess you could log the values, or just skip the logging in that function, and log in isConditionValid() and isTheOtherConditionValid() – BЈовић Aug 4 '15 at 12:09
  • yes, but then we still have the naming issue. – dyesdyes Aug 4 '15 at 14:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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