C++: Empty if statement

Question

This is more of an entry level question, but I'm wondering if it's good practice to have an empty if statement.

Consider this code:

void RabbitList::purge()
{
    if(head == NULL)
    {
        //cout << "Can't purge an empty colony!" << endl;
    }
    else
    {
        //Kill half the colony
        for(int amountToKill = (getColonySize()) / 2; amountToKill != 0;)
        {
            RabbitNode * curr = head;
            RabbitNode * trail = NULL;

            bool fiftyFiftyChance = randomGeneration(2);

            //If the random check succeeded but we're still on the head node
            if(fiftyFiftyChance == 1 && curr == head)
            {
                head = curr->next;
                delete curr;
                --size;
                --amountToKill;
            }
            //If the random check succeeded and we're beyond the head, but not on last node
            else if(fiftyFiftyChance == 1 && curr->next != NULL)
            {
                trail->next = curr->next;
                delete curr;
                --size;
                --amountToKill;
            }
            //If the random check succeeded, but we're on the last node
            else if(fiftyFiftyChance == 1)
            {
                trail->next = NULL;
                delete curr;
                --size;
                --amountToKill;
            }
            //If the random check failed
            else
            {
                trail = curr;
                curr = curr->next;
            }
        }
        cout << "Food shortage! Colony has been purged by half." << endl;
    }
}

As you can see, the if statement on line 5 is currently commented out; this was more of a debug text and I don't want to send any feedback to the console anymore. I'm pretty sure it would be considered bad practice to have the if statement do nothing. I know I could have return;

but since my return type is void it gives me an error. What if my return type is not void for example?

This a style question, so inherently subjective. But a good rule of thumb is to never have commented-out code lying around in your codebase. — Oliver Charlesworth, Jan 11 '13 at 01:12
@Oli Thanks for your answer. I'm learning this by my own, so since I have no direct mentoring I still prefer to ask these kind of questions. — Marcan, Jan 11 '13 at 01:14
Just a side note: using the macro `NULL` is now deprecated, prefer `nullptr`, see http://stackoverflow.com/questions/1282295 and http://stackoverflow.com/questions/13816385 — kebs, Mar 23 '14 at 18:01

score 6 · Answer 1 · answered Jan 11 '13 at 01:12

6

Even if your return type is void it is legal to return there, and certainly since the if has braces, atleast this isnt a bug waiting to happen. However it is not pretty and involves a little more work to read/understand.

You could reword it to

if(head == NULL) // or if(!head)
    return;

....

This should remove the need for the else, and the rest of the code is now inside the function and not a nested scope, a happy perk.

answered Jan 11 '13 at 01:12

Karthik T

31,456
5
68
87

This is my preference. When you have a simple function and some conditions will cause it to do nothing, then I like to detect those at the start and return. That way I avoid excessive indentation and have a policy that if I reach the end of the function, then it was successful. – paddy Jan 11 '13 at 01:59

score 5 · Answer 2 · answered Jan 11 '13 at 13:08

For a single branch, just write it directly:

if (head != 0) {
    // whatever
}

For multiple branches, sometimes having an empty first branch can simplify the conditions that follow:

if (head == 0) {
    // nothing to do
} else if (head->next == 0) {
    // whatever
} else {
    // whatever else
}

Yes, you could write that last one with an additional layer:

if (head != 0) {
    if (head->next == 0) {
        // whatever
    } else {
        // whatever else
    }
}

but the first form is clearer, especially when the second form would end up with three or four levels of if.

Oh, and

if (head == 0)
    return;

can sometimes be difficult, since it introduces an extra point of exit to the function. In the past I was a fan of this form, but in the past few years I've found that I end up removing it pretty consistently.

score 2 · Accepted Answer · answered Jan 11 '13 at 01:12

2

I would get rid of the if part, up to the first else, and replace it with if (head), which is the opposite condition, thus perfect for replacing the else in an if-else situation. However, there is then the matter of an extra tab of indentation for the entire function. At that point, it does become more preference, but I myself prefer to get the check out of the way early and not have the indentation.

If you need to return from anywhere, you can do so with return;.

answered Jan 11 '13 at 01:12

chris

60,560
13
143
205

So: if(head != NULL) AND if(head) are the one and same thing, correct? As is if(head == NULL) AND if(!head)? – Marcan Jan 11 '13 at 01:22
@Marcan, Yes, I much prefer the shorter forms. – chris Jan 11 '13 at 01:25
Thank you. What about in a longer statement thats checks 2 conditions? For example: if(mutantCount() > 0 && head). Or is it better to write: if(mutantCount() > 0 && head != NULL) for clarity? – Marcan Jan 11 '13 at 01:32
1

@Marcan, `head` should be just as clear, especially with pointers. – chris Jan 11 '13 at 01:33

score 2 · Answer 4 · answered Jan 11 '13 at 01:13

2

I don't actually believe this is subjective. Why write dead code? It is a tell-tale sign of a beginner. Instead, just check for the condition you are after and be done with it:

if(!head)
   // stuff

answered Jan 11 '13 at 01:13

Ed S.

122,712
22
185
265

score 2 · Answer 5 · answered Jan 11 '13 at 02:06

I rewrote your function by removing repetition and redundancy. It boils down reasonably small.

void RabbitList::purge()
{
    if(head == NULL) return;

    //Kill half the colony
    for(int amountToKill = (getColonySize()) / 2; amountToKill != 0;)
    {
        RabbitNode * curr = head;
        RabbitNode * trail = NULL;

        bool fiftyFiftyChance = randomGeneration(2);

        if(fiftyFiftyChance == 1 )
        {
            if( curr == head)
                head = curr->next;
            else
                trail->next = curr->next;

            delete curr;
            --size;
            --amountToKill;
        }
        else
        {
            trail = curr;
            curr = curr->next;
        }
    }
    cout << "Food shortage! Colony has been purged by half." << endl;
}

Wow, that's great. Just going through your code made me realize how I could optimize it and therefore, make it more readable. Thanks. — Marcan, Jan 11 '13 at 14:50

score 1 · Answer 6 · answered Jan 11 '13 at 01:37

As @Oli stated in a comment, this is a subjective style issue. You have two choices:

if (<something is true>) {
    // Do nothing
} else {
    // Some code goes here
}

or

if (!<something is true>) {
    // Code goes here
}

I can imagine situations where the former can be more readable than the second, especially if the condition is moderately complex.

score 0 · Answer 7 · answered Jan 11 '13 at 03:38

I would leave it.

But I would virtualize the logging (std::cout is not always useful).

struct NullLogger : public Logger
{
    virtual void log(std::string const&) {}
};

// By default use the Null Logger
// But if you need to debug just pass a useful specialization of logging.
void RabbitList::purge(Logger const& logger = NullLogger())
{
    if(head == NULL)
    {
        logger.log("Can't purge an empty colony!");
    }
    else
    {

@Lieuwe: Complete ballder-dash. – Martin York Jan 11 '13 at 22:22 — Martin York, Jan 11 '13 at 22:22

C++: Empty if statement

7 Answers7