Skip to main content
Per comment by Toby Speight, deleted first erroneous point.
Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

Implementations should refer to the corresponding interface

BST.cpp should include BST.h.

Remove extra qualifications in class member declarations

Implementations should refer to the corresponding interface

BST.cpp should include BST.h.

Remove extra qualifications in class member declarations

Remove extra qualifications in class member declarations

Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

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

Implementations should refer to the corresponding interface

BST.cpp should include BST.h.

Remove extra qualifications in class member declarations

My compiler complains:

error: extra qualification ‘BST::’ on member ‘insert’

and it is absolutely right. What it's pointing out is that this line,

void BST::insert(Packet &data);

coming as it does within the declaration of the BST class, should not have the BST:: prefix. Instead just write it like this:

void insert(Packet &data);

It might compile with your compiler (today anyway), but it's an error and should be fixed.

Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef BST_H
#define BST_H
// file contents go here
#endif // BST_H

The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. See SF.8

Don't use system("pause")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. An example:

void pause {
    std::getchar();
}

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Use constant string concatenation

The main routine currently has these lines:

cout << "-------------------------------------------------------" << endl;
cout << "Testing BST" << endl;
cout << "-------------------------------------------------------" << endl;

But you don't really need to do it that way which potentially calls the << operator six times. Instead, you could express the same thing as this:

std::cout <<
        "-------------------------------------------------------\n" 
        "Testing BST\n"
        "-------------------------------------------------------\n";

This only calls << once. The compiler automatically concatenates the string literals together.

Rethink the interface

Do interfacing programs really need for this method to be public?

void insert(Node *&p, Node *newNode);

I would say they do not, and that further, Node is an implementation detail that could be a private class within BST. Also, the zero-argument destroyTree seems entirely redundant; you already have a destructor with identical code. Here's how to make the Node private:

class BST {
    struct Node {
        Packet *data = nullptr;
        Node *rlink = nullptr, *llink = nullptr;
    };

Prefer in-class initializers to trivial constructors

As shown above, the Node class above can be quite trivially rewritten as a struct with no explicit constructor or destructor. We let the compiler provide defaults. See C.45 for details.

Decide on ownership

When a Packet is given to the tree, does the tree own it? That is, when the tree is destroyed, should the Packets also be destroyed? If the answer is yes, which is the usual case, then there should be a move constructor for Node. If the answer is no, then you should be using a std::shared_ptr or the like. In almost no case should you be passing and storing pointers to references. I would suggest looking carefully at the interfaces for standard containers such as std::vector and std::array and emulate those.

Fix the bug

The current code has a problem. When the second item with a part ID of 456 is added, we get a tree like this. In this diagram, left links are green and right links are red. The right link that points from the 456 node to itself is an error which breaks the tree. Binary trees must have no loops.
bad tree with loop