Test if a binary tree satisfies the BST property

I implemented code logic provided in the EPI book to determine if a binary tree is a BST. There were two approaches, therefore I used polymorphism for the common parts. I'm also using templates. I would appreciate code review on my use of smart pointers? Anything that I should have been doing that I'm not. I did not defined any destructors, should I?

```
#include
#include
#include
#include

using namespace std;

template
class BinarySearchNode {

public:
unique_ptr> leftChild;
unique_ptr> rightChild;

BinarySearchNode(T pData) : data(pData), leftChild(nullptr), rightChild(nullptr) {}
BinarySearchNode(T pData, unique_ptr> pLeft, unique_ptr> pRight)
: data(pData),leftChild(move(pLeft)), rightChild(move(pRight)) {}

T getData() const { return data; };

private:
T data;
};

template
class BSTSolution {

protected:
virtual bool isBSTValid(unique_ptr> const &node,T min, T max) = 0;

public:
bool isBinaryTreeValidBinaryTree(unique_ptr> const &root){
if(root == nullptr)
return false;

return isBSTValid(root, numeric_limits::min(), numeric_limits::max());
}
};

template
class BSTRecursiveSolution : public BSTSolution {

protected:
bool isBSTValid(unique_ptr> const &node,T min, T max) {

if(node == nullptr)
{
return true;
};

if(node->getData() getData() > max) {
return false;
}

if(!isBSTValid(node->leftChild, min, node->getData()) || !isBSTValid(node->rightChild, node->getData(), max))
{
return false;
}

return true;
}
};

template
class BFSSolution : public BSTSolution {

struct QEntry {
const unique_ptr>& node;
T min;
T max;
};

protected:
bool isBSTValid(unique_ptr> const &node,T min, T max) {

queue tempQueue;
tempQueue.emplace(QEntry{node,min,max});
while (!tempQueue.empty()) {
auto const current = tempQueue.f

Overall the code looks fine. There's nothing wrong with your use of smart pointers as far as I can tell. I just have some small (maybe nitpicky remarks).


I did not defined any destructors, should I?

No there's no need for them here, as the default generated destructors are fine for your scenario. The only destructor you should add is a virtual one for your BSTSolution class. But again the default generated one is just fine.

virtual ~BSTSolution() = default;

Style

There's no need to test a unique_ptr against nullptr, since it has an bool overload which already does that, so you can directly use this:

if(!root)
    return false;

You should keep your notation style of (const) references consistent. Either put the const in front of the type or after it. In the first case you should put the ampersand on the type and in the second case directly after the const.

const unique_ptr>& node
unique_ptr> const& node

You already used std::make_unique to initialize some of the child nodes in your tree. Why not also for the variables a, b and root? This way you can get rid of the new keyword altogether. Use the auto keyword so you don't need to write the type twice.

auto a = std::make_unique>(10);
auto b = std::make_unique>(20);

Design

One issue with the overall design is that your BSTSolution class only works for arithmetic or default constructable types, since they have a template initialization for std::numeric_limits (and for default constructable types it might not work correctly). However your BinarySearchNode class happily accepts all classes as a template argument. If this code is just for yourself playing around with smart pointers that's fine but in a real environment you should either make sure your BinarySearchNode is only used with arithmetic (and maybe default constructable types) (e.g. using type traits), or improve your BSTSolution so that i can work with all classes that can be ordered using operator. In the last case you have to find a generalized way to define infinity and -infinity for the first call of the iteration.

Update
One thing I forgot to mention: Whenever you are overriding a virutal method put the override keyword after the method. This makes it easier for humans to see that this is actually an overridden method and the compiler can check whether you are really overriding a virtual method. This is useful for catching typos and type differences.

bool isBSTValid(unique_ptr> const& node, T min, T max) override

StackExchange Code Review Q#159311, answer score: 3