Return to Answer

expanded `Tree` class declaration

Source Link

edited Nov 12, 2014 at 17:12

66.6k
4
118
282

Because you're writing in C++, it would make sense to have methods that operate on a class such as Tree (using my suggested rename of item) be member functions rather than separate functions. For example, when I re-implemented your code as an object, I got this:

class Tree {
public:
    Tree(char val) : value(val), children() {}
    Tree(const char src_data[][2], int len);
    bool operator<(const Tree &other) const { return value < other.value; }
    bool PushChildren(const char src_data[][2], int len);
    bool PushChildrenforAParent(const char src_data[][2], int len);
    std::ostream& printLevel(std::ostream &out, int level) const {
        for (int i=0; i<level; ++i)
            out << "   ";
        out << value << '\n';
        ++level;
        for (const auto &kid : children)
            kid.printLevel(out, level);
        return out;
    }
    friend std::ostream& operator<<(std::ostream &out, const Tree &node) {
        return node.printLevel(out, 0);
    }
private:
    char value;
    std::list<Tree> children;

    // a function to get the parent:
    static char getTopParent(const char src_data[][2], int len) {
        for (int i = 0; i < len; ++i)
            if (src_data[i][0] == '\0')
                return src_data[i][1];
        return '\0';
    }
};

Note that one of the things I added was a means to print out the tree. That kind of thing can be very handy for debugging.

Because you're writing in C++, it would make sense to have methods that operate on a class such as Tree (using my suggested rename of item) be member functions rather than separate functions.

class Tree {
public:
    Tree(char val) : value(val), children() {}
    Tree(const char src_data[][2], int len);
    bool operator<(const Tree &other) const { return value < other.value; }
    bool PushChildren(const char src_data[][2], int len);
    bool PushChildrenforAParent(const char src_data[][2], int len);
    std::ostream& printLevel(std::ostream &out, int level) const {
        for (int i=0; i<level; ++i)
            out << "   ";
        out << value << '\n';
        ++level;
        for (const auto &kid : children)
            kid.printLevel(out, level);
        return out;
    }
    friend std::ostream& operator<<(std::ostream &out, const Tree &node) {
        return node.printLevel(out, 0);
    }
private:
    char value;
    std::list<Tree> children;

    // a function to get the parent:
    static char getTopParent(const char src_data[][2], int len) {
        for (int i = 0; i < len; ++i)
            if (src_data[i][0] == '\0')
                return src_data[i][1];
        return '\0';
    }
};

Note that one of the things I added was a means to print out the tree. That kind of thing can be very handy for debugging.

Source Link

created Nov 12, 2014 at 16:23

Edward

66.6k
4
118
282

Here are some things that may help you improve your code.

Use relevant `#include`s

Your implementation requires std::list so the interface should have the line #include <list>.

Name classes wisely

The code currently uses a class named item which is a very generic word. A more descriptive name would be Tree. Also note that, while not compulsory, a common convention in C++ is to use capitalized names for classes, and lowercase names for instances of classes and other variables.

Follow a naming convention

Some of your functions, such as compareChild use camel-case, and others, such as PushChildren start with a capital letter, and others, such as initialize_tree use embedded underscores. This inconsistency makes it harder to read, understand and maintain the code. Better would be to use a convention consistently. There are many different ones out there and some, such as Google's are available on the web. Pick one and use it consistently; consistent use is more important than which particular convention you use.

Use object orientation

Because you're writing in C++, it would make sense to have methods that operate on a class such as Tree (using my suggested rename of item) be member functions rather than separate functions.

Avoid using pointers

Modern C++ doesn't really need pointers very often. It's usually better to either use a smart pointer, such as std::unique_ptr or simply use objects or object references. For this code, that suggests that instead of children being a std::list<Tree*> that they would probably be better as std::list<Tree>.

Avoid `new` and `delete`

If you don't use pointers, you have much less reason to use new and delete. But if you do use new, each instance of new must be matched to a corresponding delete. Often the best way to do this is to put delete in a destructor.

Prefer `class` to `struct`

The only real difference, of course, is that by default, the members of a struct are public, while the members of a class are private. Still, it's best to keep the internals of a class private to reduce linkage among objects to only what they need. This simplifies the interface and therefore the maintenance. If, for example, you wanted to use a std::array instead of a std::list internally to the Tree, it shouldn't require that any code using the tree is affected.