HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Binary tree with C++11 smart pointers

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withsmartbinarypointerstree

Problem

I'm trying to replace the use of raw pointers with smart pointers in my C++ code. The following bit is from my first attempt at a self-balancing binary tree, though there is nothing self-balancing at the moment. I am worried about some fundamental problems regarding the use of unique pointers.

template 
class rbtree {
    struct rbnode;
    typedef std::unique_ptr node_ptr;
    node_ptr root_;

    struct rbnode {
        node_ptr left, right;
        rbnode *parent;
        enum { Red, Black } color;
        D data;
        K key;
        rbnode(): 
            left(nullptr), right(nullptr), parent(nullptr), 
            color(Red), data(0), key(0) {}
        rbnode(K k, D d): 
            left(nullptr), right(nullptr), parent(nullptr), 
            color(Red), data(d), key(k) {}
        rbnode(K k, D d, rbnode *p): 
            left(nullptr), right(nullptr), parent(p), 
            color(Red), data(d), key(k) {}
    };

    node_ptr insert_(node_ptr& node, node_ptr& parent, K& key, D& data);
    public:

        bool insert(K& key, D& data);
        void dfs(std::function visitor);
};

template 
void rbtree::dfs(std::function visitor) {
}

template 
typename rbtree::node_ptr rbtree::insert_(
        node_ptr& node, node_ptr& parent, K& key, D& data) {
    if (node == 0) { node = node_ptr(new rbnode(key, data, parent.get())); }
    if (key == node->key) { throw "Key exists!"; }
    if (key key) {
        node->left = std::move(insert_(node->left, node, key, data));
    } else {
        node->right = std::move(insert_(node->right, node, key, data));
    }
    return std::move(node);
}

template 
bool rbtree::insert(K& key, D& data) {
    root_ = std::move(insert_(root_, root_, key, data));
    root_->color = rbnode::Black;
    return true;
}

Solution

Unless I missed something, it appears that bool rbtree::insert(K& key, D& data) always returns true. It is quite misleading. If there is no way for this function to ever return anything but true, you could as well get rid of the bool return type.

Also, I propose a different approach than Loki's one for the default constructors of rbnode: you can use in-class members initializers for the default values:

struct rbnode
{
    node_ptr left { nullptr };
    node_ptr right { nullptr };
    rbnode *parent { nullptr };
    enum { Red, Black } color = Red;
    D data;
    K key;

    // Etc...
};


This way, your default constructor can be reduced to its simplest form:

rbnode() = default;


And the "default" values from the in-class member initializers will also be used to automatically initialize non-initialized members in the other constructors:

rbnode(K k, D d):
    data{d},
    key{k}
{}

rbnode(K k, D d, rbnode* p):
    parent{p},
    data{d},
    key{k}
{}


Note that I also used list initialization for some of the in-class members initializers and for the constructor initialization list. List initialization can help to avoid some problems related to implicit narrowing conversions of values.

Code Snippets

struct rbnode
{
    node_ptr left { nullptr };
    node_ptr right { nullptr };
    rbnode *parent { nullptr };
    enum { Red, Black } color = Red;
    D data;
    K key;

    // Etc...
};
rbnode() = default;
rbnode(K k, D d):
    data{d},
    key{k}
{}

rbnode(K k, D d, rbnode* p):
    parent{p},
    data{d},
    key{k}
{}

Context

StackExchange Code Review Q#51111, answer score: 6

Revisions (0)

No revisions yet.