Skip to main content
Add details on smart pointers
Source Link
AlexV
  • 7.3k
  • 2
  • 24
  • 47

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

There is also a small memleak in insert since those Node*s created with Node *newNode = new Node; will never get deleted. valgrind confirms this. As @TobySpeight rightfully pointed out in his comment and detailed in his answer, one can argue if leaking all of the tree's nodes should really be considered as a "small" memleak. A possible solution for this is to use smart pointers from <memory>. There is a good overview in this blog post by Herb Sutter on which type of smart pointer (e.g. std::unique_ptr or std::shared_ptr) as well as the type of parameter passing (by-reference vs. by-value) should be used to express a certain "meaning".

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

There is also a small memleak in insert since those Node*s created with Node *newNode = new Node; will never get deleted. valgrind confirms this. As @TobySpeight rightfully pointed out in his comment and detailed in his answer, one can argue if leaking all of the tree's nodes should really be considered as a "small" memleak.

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

There is also a small memleak in insert since those Node*s created with Node *newNode = new Node; will never get deleted. valgrind confirms this. As @TobySpeight rightfully pointed out in his comment and detailed in his answer, one can argue if leaking all of the tree's nodes should really be considered as a "small" memleak. A possible solution for this is to use smart pointers from <memory>. There is a good overview in this blog post by Herb Sutter on which type of smart pointer (e.g. std::unique_ptr or std::shared_ptr) as well as the type of parameter passing (by-reference vs. by-value) should be used to express a certain "meaning".

added 325 characters in body
Source Link
AlexV
  • 7.3k
  • 2
  • 24
  • 47

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

There is also a smallsmall memleak in insert since those Node*s created with Node *newNode = new Node; will never get deleted. valgrind confirms this. As @TobySpeight rightfully pointed out in his comment and detailed in his answer, one can argue if leaking all of the tree's nodes should really be considered as a "small" memleak.

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

There is also a small memleak in insert since those Node*s created with Node *newNode = new Node; will never get deleted. valgrind confirms this.

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

There is also a small memleak in insert since those Node*s created with Node *newNode = new Node; will never get deleted. valgrind confirms this. As @TobySpeight rightfully pointed out in his comment and detailed in his answer, one can argue if leaking all of the tree's nodes should really be considered as a "small" memleak.

added 193 characters in body
Source Link
AlexV
  • 7.3k
  • 2
  • 24
  • 47

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

There is also a small memleak in insert since those Node*s created with Node *newNode = new Node; will never get deleted. valgrind confirms this.

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

There is also a small memleak in insert since those Node*s created with Node *newNode = new Node; will never get deleted. valgrind confirms this.

Source Link
AlexV
  • 7.3k
  • 2
  • 24
  • 47
Loading