CODEWITHSUNDEEP
c++coding-style
Str-Gen
Str-Gen

Reputation: 33

Non-void function potential lack of return

I wrote this piece of code to check if a node is a left or right child in a tree. However from a cleanliness perspective, I don't think it is properly implemented.

My issue is that returning a default like false if the parent pointer is null, is dangerous because false would mean it's a right child. However this function needs a return if I want it to compile without warnings. The empty throw is definitely ugly as a solution.

I have seen people use assert(parent) instead of if, but I don't think that's good either because the assert a debug feature.

In short, what are better ideas to implement this function? 

bool isLeftChild() const {
    if(parent){
        if(this == (parent->left).get()){
            return true;
        }
        else if(this == (parent->right).get()){
            return false;
        }
    }
    else throw;
}

Upvotes: 0

Views: 59

Answers (2)

Michael Kenzel
Michael Kenzel

Reputation: 15943

I guess it depends on what you want. I would argue that a function called isLeftChild() should return false in case the node is not anyone's child, because if it's no one's child, it's definitely not a left child…

Concerning the use of assert(), you should ask yourself the question: is it necessary to guarantee well-defined behavior for a case where the function is incorrectly called? The point of assert() is to catch situations that should never occur in a correct program. Once you build your release version, these checks should not be necessary anymore, because the situations they check for can (should) never occur. Also, note that your throw expression without an argument will simply call std::terminate() unless there is an active exception being handled. I doubt that isLeftChild() is intended to be called exclusively only from inside catch blocks!? So for this throw to do what you most likely intended to do, you need to throw something, for example an std::logic_error.

Upvotes: 1

Nicol Bolas
Nicol Bolas

Reputation: 473232

Cleanliness is ultimately a matter of opinion. What you're having trouble with is the question of what this function's contract is. That is, do you want it to define the behavior of this code if it is called on a node with a NULL parent?

If the answer is yes, then throwing is perfectly valid. Much like vector::at throws if the index is out of bounds.

bool isLeftChild() const
{
    if(!parent) throw ...;

    if(this == (parent->left).get()){
        return true;
    }
    else if(this == (parent->right).get()){
        return false;
    }
}

If the answer is no, then you shouldn't throw. This is how vector::operator[] works with an out-of-bounds index. In C++20, you would express this as a contract:

bool isLeftChild() const
    [[expects: parent != nullptr]]
{
    if(this == (parent->left).get()){
        return true;
    }
    else if(this == (parent->right).get()){
        return false;
    }
}

But this is conceptually an assert; you are saying that undefined behavior results if you call this function on a node with a NULL parent.

Which is "cleaner" is up to you and how you want your interface to behave. "Wide contracts" (where you throw on failure rather than invoke UB) are often seen as the "safer" alternative. This is primarily true in that you get an exception thrown from the place where things went wrong, rather than from other locations. But in this particular case, if parent is NULL, you'll immediately find out when you try to dereference it.

But even so, wide contracts are generally considered poor form in modern C++. Indeed, with contracts in C++20, the committee is looking into slowly removing the places in the standard library that throw logic_errors, turning them instead into contract violations (ie: UB).

Upvotes: 1

Related Questions