1
\$\begingroup\$

I'm returning to C++ after more than 10 years. The code I wrote for a simple quadtree style class has a ton of new elements I haven't used. I'm looking for comments on the design and not necessarily the quadtree implementation.

The code in its current state will work for my needs, with the exception of error catching. Can I improve it? Am I making any design mistakes?

#include <array>

template <typename T>
class Quadtree
{
public:
    typedef std::array<Quadtree, 4> Quadrants; // class scope typedef

    T data; // the template data
    Quadtree* parent;

    Quadtree(T _data) // constructor
    {
        data = _data;
    }

    void Split(Quadrants nodes)
    {
        quadrants = &nodes;

    }

    Quadtree operator[](int _index) // keeps syntax consistent with std::array
    {
        return (*quadrants)[_index];
    }
private:
    Quadrants* quadrants;
};

Usage looks like this:

Quadtree<int> tree(5);
Quadtree<int> node(3);
Quadtree<int>::Quadrants quadrant = { *new Quadtree<int>(1), *new Quadtree<int>(2), *new Quadtree<int>(4), node };
tree.Split(quadrant);

int value = tree[3].data;
// value now equals 89
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Split takes a reference to a value on the stack which will be undefined after leaving the function. You might want to use std::shared_ptr or std::unique_ptr. \$\endgroup\$
    – Christoph
    Commented Feb 2, 2017 at 6:55
  • \$\begingroup\$ The code contains undefined behavior as mentioned by Cristoph. Did you run tests? By the way, you can fix it if you pass it by pointer and not by value. Also, design looks as a mix of C and Java/C#. \$\endgroup\$ Commented Feb 2, 2017 at 8:53
  • \$\begingroup\$ Whoops, breaking Split was an unintended edit to the code. When I typedef'd Quadrants I messed it up. Unfortunately it works because the memory isn't "freed" right away. I'm testing in VS C++ right now, but the codebase will target GCC at a later time. \$\endgroup\$ Commented Feb 3, 2017 at 3:44

1 Answer 1

2
\$\begingroup\$

It actually works...

After thinking a bit I believe I figured out why this thing works.

This paragraph is inteded purely for people who thinks that the question is invalid due to broken code.

It is invalid. Yes. In terms of standard C++. But undefined behavior doesn't mean that the behavior can be defined on some platform when using some compiler. By defined I don't mean accident. By defined I mean reproducible and consistent behavior. Compiler can do copy elision from call site to function site, so that the function will receive the same thing in terms of memory location as on the call site!

But it is non standard conformant compiler. Or is it? The thing is that taking the address of the local variable and using it somewhere else is undefined behavior, so why miss such a great opportunity to optimize code? I believe these are the thoughts that came to mind of the compiler implementer.

May be it is possible to verify that using disassembly.

Code Review:

As I mentioned in the comment, the design is strange mix of C and Java/C#. Class is used, but parent and data are left public. Data is taken by value (or maybe by reference was intended?). So there are lots of strange things happening here aside from the undefined behavior.

Ambiguous model

After a few minutes thinking I couldn't find out what is meant by the Quadtree.

Obsolete design

C++ now has smart pointers. I've seen on cppcon 2016 unique_ptr<> compiling down to the version with raw pointers. It was simple case, but I believe more complex cases will behave the same. It might complicate design a bit, but it will make it leak free by default (well, the program can crash though, due to double free, but it is better than memory leak in my opinion).

using declarations are now preferred to typedefs.

using Quadrants = std::array<Quadtree, 4>;

No clear preference of value/reference

The array is stored by pointer. I know that may be copy elision is intended, but the code leaves lifetime management of the nodes to the caller, which potentially makes the usage very hard and error prone. Famous Scott Meyers said: "Make interfaces easy to use correctly and hard to use incorrectly". Very sound advice and should be kept in mind when designing public interfaces.

There is a thing called move constructor in C++11, but on std::array<> it will behave the same as if it is by element copy, so rip it. Some wrapper over std::vector<> might be preferred.

Non idiomatic C++

In general, I would say that the code doesn't look like C++ in the form that I see here or on the web by acknowledged people. Though I would say that it is also important to know when idiomatic C++ can't do the job wanted, but idiomatic C++ should be preferred if it possible.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.