1
\$\begingroup\$

I'm here looking for a review over my current code-base.

Please:

  • Ignore using namespace std; this is for educational purposes mostly so the usage of using namespace std; does not matter for this codebase as it's a project thrown together for educational purposes.

  • Look at some parts of my code which I will point out below and if possible, look at the rest giving me feedback and telling me how to possibly improve/showing me.

In my InventorySystem, I have code which reads into a text-file that's structured like so:

{
123,
A piece of cheese.,
10.19,
4,
}

{
321,
A country flag,
10.00,
1,
}

Specifically, it has Packet.h's structure which you can see below.

This code:

string line, line2, line3, line4;
int num1;
double num2;
int num3;
while (getline(inFile, line)) {
    getline(inFile, line);
    line = line.substr(0, line.size() - 1);
    num1 = stoi(line);
    getline(inFile, line2);
    line2 = line2.substr(0, line2.size() - 1);
    getline(inFile, line3);
    line3 = line3.substr(0, line3.size() - 1);
    num2 = stod(line3);
    getline(inFile, line4);
    line4 = line4.substr(0, line4.size() - 1);
    num3 = stoi(line4);
    Packet importPacket(num1, line2, num2, num3);
    inventorySystem.insert(importPacket);
    getline(inFile, line);
    getline(inFile, line);
}
inFile.close();

In the beginning of int main() in Inventory.cpp is supposed to read into it section by section. But looking at the code, as you can see it seems very clunky. Do you have any suggestions for improving it? Note that it tosses out the "," seen at the end of every attribute so that it can read the id, description, price, partCount of Packet.h.

There's also this part

else if (choice == 'a') {
    cout << "Archiving all the information." << endl;
    Sleep(1000);
    cout << "Archiving all the information.." << endl;
    Sleep(1000);
    cout << "Archiving all the information..." << endl;
    vector <Packet*> getPackets = inventorySystem.extract();

    bOutFile.open("C:\\...Put_Your_Path_Here.dat", ios::out | ios::binary);
    for (int i = 0; i < getPackets.size(); ++i) {
        bOutFile.write((char*)&getPackets[i], sizeof(getPackets[i]));
    }
    bOutFile.close();
}

For archiving to binary .dat purposes where I use the extract function on the binary search tree to grab all of its contents and put them into a vector for archiving into .dat purposes. I'm not sure if this is a good way of doing so. Could it be improved? I want to extract still so that I don't have to write more code if possible. This code also uses pretty much the preorder traversal in order to extract contents of BST. It's pretty repetitive. This code can be found in int main() at the if...choice "a." Here's how it executes from BST.cpp:

vector <Packet*> BST::extract() const {
    vector <Packet*> result;

    if (root != nullptr) {
        extract(root, result);
    }

    return result;
}

void BST::extract(const Node *p, vector <Packet*> &result) const {
    if (p != nullptr) {
        result.push_back(p->data);
        extract(p->llink, result);
        extract(p->rlink, result);
    }
}

It's recursive. That's all. But if you can look at other parts of the codebase that'd be really great too!

This part for reference purposes:


Inventory.cpp:

#include <iostream>
#include <string>
#include <fstream>
#include "windows.h" 

#include "BST.h"
#include "Packet.h"

using namespace std;

/*
    PURPOSE: Process inventory information for a parts warehouse.
    1. Read from txt. file used for storing information between sessions.
        *Txtfile can be empty.
    2. Pop up 5 choices in main menu: enter a new part to system, find and print data for a part when given the part ID number,
    and find and modify data for a part when given the part ID number, copy all information to a binary archive file, and quit.
*/

int main() {
    BST inventorySystem;
    ifstream inFile("C:\\...Put_Your_Path_Here.txt");
    ofstream outFile;
    fstream bOutFile;

    if (!inFile) {
        cerr << "ERROR: Unable to open the text file." << endl;
    }
    else {
        string line, line2, line3, line4;
        int num1;
        double num2;
        int num3;
        while (getline(inFile, line)) {
            getline(inFile, line);
            line = line.substr(0, line.size() - 1);
            num1 = stoi(line);
            getline(inFile, line2);
            line2 = line2.substr(0, line2.size() - 1);
            getline(inFile, line3);
            line3 = line3.substr(0, line3.size() - 1);
            num2 = stod(line3);
            getline(inFile, line4);
            line4 = line4.substr(0, line4.size() - 1);
            num3 = stoi(line4);
            Packet importPacket(num1, line2, num2, num3);
            inventorySystem.insert(importPacket);
            getline(inFile, line);
            getline(inFile, line);
        }
        inFile.close();


        char choice = 'z';

        while (choice != 'q') {
            cout << "-----------------------------------------------------" << endl;
            cout << "Program successfully loaded..." << endl;
            cout << "Welcome to the main menu..." << endl;
            cout << "-----------------------------------------------------" << endl;
            cout << "N: Enter new part into the system >>" << endl;
            cout << "F: Find a particular part >>" << endl;
            cout << "A: Archive the information >>" << endl;
            cout << "Q: Quit >>" << endl;
            cout << "Input your choice: ";
            cin >> choice;
            choice = tolower(choice);

            if (choice == 'n') {
                cout << "Enter the new part's I.D.: ";
                int partId = -1;
                cin >> partId;
                cin.ignore();; // Flushes the input stream and removes the new line(s) at the end of the stream for the upcoming getline.
                cout << "Enter a short description for this new part: ";
                string description = "";
                getline(cin, description);
                cout << "Enter the price of this new part: $";
                double price = 0.00;
                cin >> price;
                cout << "Enter how many parts the warehouse currently has: "; 
                int partCount = 0;
                cin >> partCount;

                if (partId >= 0 && price >= 0 && partCount >= 0) {
                    Packet packet(partId, description, price, partCount);
                    inventorySystem.insert(packet);
                    cout << "Attempted to enter the part into the SYSTEM." << endl;
                }
                else {
                    cout << "One or more inputs are invalid. You will be prompted back to the menu. Please enter a valid input!" << endl;
                    cout << "Usage: PartID should be greater than or equal to 0, price should be similar, and vice versa!" << endl;
                }
            }
            else if (choice == 'f') {
                cout << "Enter the part I.D. that you want to search the database for: ";
                int partId = -1;
                cin >> partId;
                Packet* getPacket = inventorySystem.search(partId);

                if (getPacket != nullptr) {
                    cout << "{" << endl;
                    cout << "I.D.: " << getPacket->getPartId() << endl;
                    cout << "Description: " << getPacket->getDescription() << endl;
                    cout << "Price: " << getPacket->getPrice() << endl;
                    cout << "Part Count: " << getPacket->getPartCount() << endl;
                    cout << "}" << endl;
                }
                else {
                    cout << "ERROR: System could not find the following I.D. as part of the inventory system." << endl;
                }
            }
            else if (choice == 'a') {
                cout << "Archiving all the information." << endl;
                Sleep(1000);
                cout << "Archiving all the information.." << endl;
                Sleep(1000);
                cout << "Archiving all the information..." << endl;
                vector <Packet*> getPackets = inventorySystem.extract();

                bOutFile.open("C:\\...Put_Your_Path_Here.dat", ios::out | ios::binary);
                for (int i = 0; i < getPackets.size(); ++i) {
                    bOutFile.write((char*)&getPackets[i], sizeof(getPackets[i]));
                }
                bOutFile.close();
            }
        }

        vector <Packet*> getPackets = inventorySystem.extract();

        outFile.open("C:\\...Put_Your_Path_Here.txt");
        for (int i = 0; i < getPackets.size(); ++i) {
            outFile << "{" << endl;
            outFile << getPackets[i]->getPartId() << "," << endl;
            outFile << getPackets[i]->getDescription() << "," << endl;
            outFile << getPackets[i]->getPartCount() << "," << endl;
            outFile << getPackets[i]->getPrice() << "," << endl;
            outFile << "}" << endl << endl;
        }
        outFile.close();
    }
    system("pause");
}

BST.h:

#pragma once

#include "Packet.h"
#include <vector>

using namespace std;

class BST {
    struct Node {
        Node() : rlink(nullptr), llink(nullptr) {};
        ~Node() {};

        Packet *data;
        Node *rlink, *llink;
    };
public:
    BST();
    void insert(Packet &p);
    void insert(Node *&p, Node *newNode);
    Packet* search(int id);
    vector <Packet*> extract() const;
    void preorderTraversal() const;
    void destroyTree();
    ~BST();
private:
    Node * root;
    void destroyTree(Node *&p);
    Packet* search(const Node *p, int id);
    void extract(const Node *p, vector <Packet*> &result) const;
    void preorderTraversal(const Node *p) const;
};

BST.cpp:

#include "BST.h"
#include <iostream>

BST::BST() : root(nullptr) {}

void BST::insert(Packet &p) {
    if (search(p.getPartId()) == nullptr) {
        Node *newNode = new Node;
        newNode->data = new Packet(p);
        insert(root, newNode);
    }
    else {
        cout << "Insertion failed because such packet has already been found to exist." << endl;
    }
}

void BST::insert(Node *&p, Node *newNode) {
    if (p == nullptr) {
        p = newNode;
    }
    else if (p->data->getPartId() > newNode->data->getPartId()) {
        insert(p->llink, newNode);
    }
    else {
        insert(p->rlink, newNode);
    }
}

Packet* BST::search(int id) {
    return search(root, id);
}

Packet* BST::search(const Node *p, int id) {
    if (p == nullptr) {
        return nullptr;
    }
    else if (p->data->getPartId() == id) {
        return p->data;
    }
    else if (p->data->getPartId() < id) {
        return search(p->rlink, id);
    }

    return search(root->llink, id);
}

vector <Packet*> BST::extract() const {
    vector <Packet*> result;

    if (root != nullptr) {
        extract(root, result);
    }

    return result;
}

void BST::extract(const Node *p, vector <Packet*> &result) const {
    if (p != nullptr) {
        result.push_back(p->data);
        extract(p->llink, result);
        extract(p->rlink, result);
    }
}

void BST::preorderTraversal() const {
    if (root == nullptr) {
        cerr << "There is no tree.";
    }
    else {
        preorderTraversal(root);
    }
}

void BST::preorderTraversal(const Node *p) const {
    if (p != nullptr) {
        cout << p->data->getPartId() << " ";
        preorderTraversal(p->llink);
        preorderTraversal(p->rlink);
    }
}

void BST::destroyTree(Node *&p) {
    if (p != nullptr) {
        destroyTree(p->llink);
        destroyTree(p->rlink);
        delete p;
        p = nullptr;
    }
}

void BST::destroyTree() {
    destroyTree(root);
}

BST::~BST() {
    destroyTree(root);
}

Packet.h:

#pragma once
#include <string>

using namespace std;

class Packet {
    public:
        Packet(int partId, string description, double price, int partCount) : 
            partId(partId), description(description), price(price), partCount(partCount) {}
        int getPartId() const {return partId;}
        string getDescription() const {return description;}
        double getPrice() const {return price;}
        int getPartCount() const {return partCount;}

    private:
        int partId;
        string description;
        double price;
        int partCount;
};
\$\endgroup\$
  • 2
    \$\begingroup\$ It's OK to ask us to ignore something, but using namespace std; is for "educational purposes only"? I'd say you should particularly avoid them for education purposes. \$\endgroup\$ – L. F. Jul 7 at 1:29
  • \$\begingroup\$ It would most likely not be used if it's reasonable in a real project. I may have worded it wrong but yeah, please ignore it. \$\endgroup\$ – ii69outof247 Jul 7 at 3:24
  • \$\begingroup\$ @ii69outof247 If you put code for review here you should be open for any aspect of your code being reviewed. You can't simply exclude certain aspects should be ignored by reviewers. Especially using namespace std is a big red flag which should be improved. \$\endgroup\$ – πάντα ῥεῖ Jul 7 at 7:13
  • \$\begingroup\$ @ii69outof247 See here also: codereview.meta.stackexchange.com/questions/5773/… \$\endgroup\$ – πάντα ῥεῖ Jul 7 at 7:18
1
\$\begingroup\$

I see a number of things that may help you improve your code.

Prefer portable code to platform specific code

The code is currently calling Sleep which is a platform-dependent function. This could be made platform independent by instead using std::this_thread::sleep_for

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 PAUSE 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 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++. For example:

void pause() {
    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. It is particularly bad to put it into a header file, so please don't do that.

Consider the user

Instead of having a hardcoded filename, it might be nice to allow the user to control the name and location of the input and output files. For this, it would make sense to use a command line argument and then pass the filename to the functions as needed.

Use better names

It's baffling that the class containing a part is called Packet instead of Part. Good names are extremely useful in creating and maintaining good code.

Be careful with signed and unsigned

In two cases, the code compares an int i with getPackets.size(). However, getPackets.size() is unsigned and i is signed. For consistency, it would be better to declare i as std::size_t which is the type returned by size().

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 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

Fix the bug

The current archiving is mostly useless since it writes memory pointer values to the output file. After the program ends, those pointer values are going to be completely useless. What was apparently intended was to write a binary representation of the part, but that's not what's currently happening.

Be careful with raw pointers

The BST::extract returns a vector of raw pointers to internal data. This is almost certainly going to be a problem because there is nothing to prevent the BST from being deleted (invalidating all of those pointers) before the returned vector is used. This is not a good design. Better would be instead to allow direct traversal of the BST data structure via iterators. This is safer and much easier to use in conjunction with standard algorithms.

Prefer a stream inserter to a custom print routine

Your main routine currently writes all of the detailed inventory data. Instead, it could be written as a stream inserter:

friend std::ostream& operator<<(std::ostream &out, const Packet &p) {
    return out << "{\n" << p.partId << ",\n" << p.description 
        << ",\n" << p.partCount << ",\n" << p.price << "\n}\n\n";
}

Then the loop in main could be this:

for (std::size_t i = 0; i < getPackets.size(); ++i) {
    outFile << getPackets[i];
}

Or even better:

std::copy(getPackets.begin(), getPackets.end(), 
        std::ostream_iterator<Packet>(outFile));

Note that both versions assume that getPackets is a vector of Packet rather than a vector of pointers as mentioned in the previous point. A similar thing can be done to create an extractor using operator>>.

Don't use console I/O for errors

The BST class writes to std::cerr or std::cout (it would be nice to be consistent!) when it encounters an error. Better would be to return a flag indicating success or perhaps throwing an exception.

Don't define redundant functions

The destroyTree() function is exactly a duplicate of the destructor. Since the destructor must be present, the destroyTree function should be omitted because it is redundant.

Don't define a default constructor that only initializes data members

The BST::Node constructor is currently this:

Node() : rlink(nullptr), llink(nullptr) {};

Better would be to use in-class member initializers. See C.45

Don't define an empty destructor

The current ~Node is empty. Better would be to simply omit it. See C.30.

Be careful with interfaces

The void BST::insert(Node *&p, Node *newNode); function should be private because it requires the use of a Node object.

Don't leak memory

At the moment, the data pointer of each BST::Node is never freed which is a memory leak. See C.31

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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