patterncppMinor
BST C++ STL implementation, visiting algorithms
Viewed 0 times
stlbstvisitingalgorithmsimplementation
Problem
I am currently learning C++, and as an exercise I tried to implement a BST in a C++11. I am not sure at all if what I have done could be considered a good example of c++ programming, thus I would love to know from you if you believe my code could be improved in terms of functionalities, efficiency, or just programming style.
In particular, these are my doubts:
I had a look at others implementations that I was able to find on the web, in order to correct my first (and I must admit too much C-like first attempt).
```
template class BinarySearchTree {
private:
// nested class for the node of the tree
class TreeNode {
public:
std::unique_ptr left;
std::unique_ptr right;
T value;
TreeNode(T val) {
value = val;
}
};
// private attributes / methods of BST class
std::unique_ptr root; // root of the BST
int V; // number of nodes
void private_recursive_inorderVisit(TreeNode *node) {
if(node == nullptr)
return;
private_inorderVisit(node->left.get());
std::cout value right.get());
}
void private_iterative_inorderVisit(TreeNode *node) {
TreeNode *currNode = node;
std::stack s;
while(s.size() > 0 || currNode != nullptr) {
if(currNode != nullptr) {
s.push(currNode);
currNode = currNode->left.get();
} else {
currNode = s.top();
In particular, these are my doubts:
- Is the use I made of
unique_ptra good idea in this situation? (It should be because of RAII, from what I got.)
- Is the use of nested class a good idea in this situation, or even in general? In this case I think that maybe it could not be a problem because all its members are public but the class itself is encapsulated within the
BinarySearchTree, so the inner class is not visible from outside theBinarySearchTreeclass.
- I am not really sure about the checkBST function. Could it be better?
I had a look at others implementations that I was able to find on the web, in order to correct my first (and I must admit too much C-like first attempt).
```
template class BinarySearchTree {
private:
// nested class for the node of the tree
class TreeNode {
public:
std::unique_ptr left;
std::unique_ptr right;
T value;
TreeNode(T val) {
value = val;
}
};
// private attributes / methods of BST class
std::unique_ptr root; // root of the BST
int V; // number of nodes
void private_recursive_inorderVisit(TreeNode *node) {
if(node == nullptr)
return;
private_inorderVisit(node->left.get());
std::cout value right.get());
}
void private_iterative_inorderVisit(TreeNode *node) {
TreeNode *currNode = node;
std::stack s;
while(s.size() > 0 || currNode != nullptr) {
if(currNode != nullptr) {
s.push(currNode);
currNode = currNode->left.get();
} else {
currNode = s.top();
Solution
Okay some minor things here:
As I mentioned in a comment:
Naming
Why did you give your private methods the prefix
One of your methods is called
Pointers
Next thing:
This line of code obviously works, but generally it is recommended to use
Also you are mixing
Nested class
I rarely ever use nested classes, but your case here seems like it's worth an exception. This inner class is only used inside the outer class and has no sense whatsoever outside of the class. So making it an inner class seems like the way to go.
As I mentioned in a comment:
Naming
Why did you give your private methods the prefix
private_ ? Looks code noise to me. Also these methods use cout. You should consider using an ostream-parameter (that may, or may not be defaulted to cout) instead. This way you could let the caller of the method decide whether he wants the output in a console, or a file, or sent to the international space station.One of your methods is called
inorderVisit. Consider calling it visitInOrder as method names should generally start with a verb.Pointers
Next thing:
std::unique_ptr node(new TreeNode(RootValue));This line of code obviously works, but generally it is recommended to use
make_unique instead:std::unique_ptr node = std::make_unique(RootValue);Also you are mixing
unique_ptr with raw pointers. Consider using weak_ptr or shared_ptr where necessary.Nested class
I rarely ever use nested classes, but your case here seems like it's worth an exception. This inner class is only used inside the outer class and has no sense whatsoever outside of the class. So making it an inner class seems like the way to go.
Code Snippets
std::unique_ptr<TreeNode> node(new TreeNode(RootValue));std::unique_ptr<TreeNode> node = std::make_unique<TreeNode>(RootValue);Context
StackExchange Code Review Q#100855, answer score: 2
Revisions (0)
No revisions yet.