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