CODEWITHSUNDEEP
c++
Isaac
Isaac

Reputation: 115

Construction convention for multiple if method

I wounder if there is some construction convention for constructing method with multiple if's. Most of the time before you fire your method you have to check input arguments and other things, eg. is not nullptr, is > 0, is != -1 etc. Sometimes you cannot check this in one if and as a result you have something like this:

if(arg != nullptr)
{
    if()
    {
        if()
        {
            if()
            {
                /*actual code here*/
            }
            else
            {

            }
        }
        else
        {

        }
    }
    else
    {
        /* other error message like "License check error: wrong key!" */
    }
}
else
{
    /* wrong input args! */
}

Good convention is that your line has less than 80 characters which gives us less space for actual code. Code is getting more and more unreadable.

Upvotes: 0

Views: 72

Answers (4)

philsumuru
philsumuru

Reputation: 234

Your original construction could be written like this:

do
{
    if(nullptr == arg) // Note: *negate* your original conditions!
    {
        /* wrong input args! */
        break;
    }
    if(...)
    {
        /* other error message like "License check error: wrong key!" */
        break;
    }
    if(...)
    {
        ...
        break;
    }
    if(...)
    {
        ...
        break;
    }

    /*actual code here*/

} while (0);

  • Advantages:

    1. no nested ifs;
    2. use break instead of goto to jump out of the whole block;
    3. the logic is clearer, and more maintainable: if you want to add a check guard, just append one more if(...){...; break;};

  • Disadvantages:

    1. do-while(0) looks a bit strange;
    2. you should negate all your original conditions, e.g. if(cond) => if(!cond), which may affect the code clarity;

Upvotes: 0

Andrej Adamenko
Andrej Adamenko

Reputation: 1668

When you have too many sub-levels of if, while, for in a function, it is a sign that the function should be split into 2 or more separate functions. Depending on specific code it could look something like this:

public void MyClass::Run(arg)
{
    if(arg != nullptr)
    {
        if()
        {
            RunActualCode()
        }
        else
        {
            /* other error message like "License check error: wrong key!" */
        }
    }
    else
    {
        /* wrong input args! */
    }
}

private void MyClass::RunActualCode(...)
{
    if()
    {
        if()
        {
            /*actual code here*/
        }
        else
        {

        }
    }
    else
    {

    }
}

There are many recommendation about this, for example:

Rec 4.7 Do not have too complex functions.

Everyone that has ever had to take over code written by someone else knows that complex code is hard to maintain. There are many ways in which a function can be complex, such as the number of lines of code, the number of parameters, or the number of possible paths through a function. The number of possible paths through a function, which is the result from the use of many control flow primitives, is the main reason to why functions are complex. Therefore you should be aware of the fact that heavy use of control flow primitives will make your code more difficult to maintain. http://www.tiobe.com/content/paperinfo/CodingStandards/hem/industrial/bookindex.htm

Limiting complexity during development

Upvotes: 0

Jan Rüegg
Jan Rüegg

Reputation: 10055

What I usually do is something like this:

if(arg == nullptr)
{
    /* wrong input args! */
    return;
}


if()
{
    /* other error message like "License check error: wrong key!" */
    return;
}

...


/*actual code here*/

Then you have all your error "ifs" and error handling in one place, and the actual function code at the end, nicely separated.

Upvotes: 1

TartanLlama
TartanLlama

Reputation: 65620

You could return early in the case of issues, or throw an exception:

if(arg == nullptr) {
    log("arg was null, not doing anything");
    return;
}

//if the user forgot to make the toast, we can do it for them
if(forgotToMakeToast) {
    makeToast();
}

if(ranOverDog) {
    //we can't continue if the user ran over our dog, throw an exception
    throw too_angry_exception;
}

//actual code

This makes your code structure more obvious by relating the error handling to the error checking by locality.

Upvotes: 3

Related Questions