CODEWITHSUNDEEP
c++if-statement
Marcan
Marcan

Reputation: 152

C++: Empty if statement

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?

Upvotes: 4

Views: 25168

Answers (7)

Pete Becker
Pete Becker

Reputation: 76245

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.

Upvotes: 6

Loki Astari
Loki Astari

Reputation: 264381

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
    {

Upvotes: 0

paddy
paddy

Reputation: 63471

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

Upvotes: 2

Code-Apprentice
Code-Apprentice

Reputation: 83527

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.

Upvotes: 1

Ed Swangren
Ed Swangren

Reputation: 124642

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

Upvotes: 2

Qaz
Qaz

Reputation: 61910

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

Upvotes: 2

Karthik T
Karthik T

Reputation: 31952

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.

Upvotes: 6

Related Questions