5
\$\begingroup\$

I'm really just looking for some advice on my current code and with moving forward. As you can see, I have a pointer to my student list and data (which just haven't been made yet).

#include<iostream>
#include<cstdio>
#include<cstdlib>

using namespace std;

class List{ //Building the base of the program, being the nodes, and initializing the functions of the list
    private:
        typedef struct node {
            int student_number;
            int class_number;
            struct node* classpointer;
            struct node* studentpointer;
        }* nodePtr; //condensing node structure to simply "nodePtr"

        nodePtr head;
        nodePtr curr;
        nodePtr temp;

    //functions that I will be using
    public:
        List();
        void addNode();
        void PrintList();
};
//initialize the list pointers
List::List() {
    head = NULL;
    curr = NULL;
    temp = NULL;
}
//function to add a node to the linked list
void List::addNode() {
    nodePtr n = new node;
    n->classpointer = NULL;

    cout << "What value would you like to add?" << endl;
        int x;
    cin >> x;
    n->class_number = x;

    if(head != NULL) {
        curr = head;
        while (curr->classpointer != NULL) {
            curr = curr->classpointer;
        }
        curr->classpointer = n;
    }
    else {
        head = n;
    }
}

void List::PrintList() {
    curr = head;
    while(curr != NULL) {
        cout << " " << endl;
        cout << curr->class_number << endl;
        curr = curr->classpointer;
    }
}

int menu() {
    int input;
    List List;
    while (input != 3) {
        cout << " " << endl;
        cout << "Press '1' to input a node" << endl;
        cout << "Press '2' to view the list of nodes" << endl;
        cout << "Press '3' to exit" << endl;
        cout << " " << endl;
        cin >> input;
        if (input == 1) {
        List.addNode();
        }
        else if (input == 2) {
        List.PrintList();
        }
        else if (input == 3) {
            return 0;
        }
        else {
        cout <<"That is an invalid key" << endl;
        }
    }
}

int main() {
    int i;
    for (i=0; i < 500; i++) {
        menu();
    }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ We can review the code as is, but we cannot assist with adding additional implementation. \$\endgroup\$
    – Jamal
    Commented Oct 1, 2015 at 21:48
  • \$\begingroup\$ Oh alright that's fine, I must have missed that rule D: \$\endgroup\$ Commented Oct 1, 2015 at 21:49
  • \$\begingroup\$ @Edward 's answer is pretty cool. But, if you wanna checkout detailed guideline of coding in Modern C++. go here. It's standard. \$\endgroup\$ Commented Oct 2, 2015 at 7:42

3 Answers 3

5
\$\begingroup\$

I see some things that may help you improve your code.

Always return an appropriate value

Your menu() routine has control paths that cause it to end without returning any int value. This is an error and should be fixed.

Separate input, output and calculation

To the degree practical it's usually good practice to separate input, output and calculation for programs like this. By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of your program (which does not depend on the underlying OS). Specifically, addNode takes no value and returns void. Within the function it asks for input and adds the node. Instead, it would be better to separate it so that you have some other input function and addNode() would take an int as an argument.

Sanitize user input better

The code doesn't quite work as posted. If I enter a string such as "Edward" to input a node, the program stays in an endless loop. It would be better to read a (text) line in and then convert it to a number. Users can do funny things and you want your program to be robust.

Use for loops rather than while loops where practical

Your PrintList routine can be considerably simplified by using a for loop rather than a while loop. here is the current code:

void List::PrintList() {
    curr = head;
    while(curr != NULL) {
        cout << " " << endl;
        cout << curr->class_number << endl;
        curr = curr->classpointer;
    }
}

You could rewrite it like this:

void List::PrintList() const {
    for (nodePtr curr = head; curr != nullptr; curr = curr->classpointer) {
        cout << " " << curr->class_number << endl;
    }
}

Note that curr is now a local variable. It doesn't need to be and shouldn't be a member variable.

Use const where practical

In the PrintList routine, the underlying List is not altered. Make this explicit by declaring that routine const as shown above.

Eliminate unused variables

In the List class, the data member temp is never used and can be eliminated.

Prefer modern initializers for constructors

The constructor use the more modern initializer style rather than the old style you're currently using. Instead of this:

List::List() {
    head = NULL;
}

You could use this:

List::List() : head{nullptr} 
{}

There is not a significant difference in this code, but it's a good habit to get into using.

Match new with delete

If you allocate memory using new, you must also free it using delete or your program will leak memory. Since you use new in addNode(), you should use delete in ~List():

List::~List() 
{ 
    if (head == nullptr) 
        return;
    for (nodePtr temp=head->classpointer; temp; temp=head->classpointer) {
        head->classpointer = temp->classpointer;
        delete temp;
    }
    delete head;
}

Use nullptr rather than NULL

Modern C++ uses nullptr rather than NULL. See this answer for why and how it's useful.

Don't abuse using namespace std

Especially in a very simple program like this, there's little reason to use that line. Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Declare variables as late as possible

Rather than using the old C-style of declaring all variables at the top of a function, use the modern C++-style and declare variables as late as possible. Doing so can sometimes help the compiler figure out register allocation, resulting in faster, smaller code. Your main could, for example, be rewritten like this:

int main() {
    for (int i=0; i < 500; i++) {
        menu();
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ In the menu() function, what values does it need to be returning, I get that it should be returning something, but I didn't know what. \$\endgroup\$ Commented Oct 2, 2015 at 0:21
  • \$\begingroup\$ Next, how do I change a text line to an in? (nvm can look that up):: For the modern initializers, I tried putting that in my program, but it didn't know what nullptr was. Is there a library that I need to include for that?:: Also, I've never heard of const, going to look it up \$\endgroup\$ Commented Oct 2, 2015 at 0:25
5
\$\begingroup\$

Unnecessary member variables

You have a member variable curr that has no purpose as a member variable, because it is only used as a temporary pointer to iterate through your list. You should make curr simply be a local variable in the functions it is used in.

The member variable temp is never used and should be deleted.

\$O(n)\$ add

Your addNode function takes linear time because it has to find the end of the list. You should either add to the beginning of the list, or keep a tail pointer so that you can quickly add to the tail.

\$\endgroup\$
2
\$\begingroup\$

The main issue you have, and which is in fact the most common issue, is memory management. As it stands, your List class is a huge crash waiting to happen (or worse).

Thus let us first focus specifically on proper memory management. I will assume a modern compiler (>= 2011) and thus the C++11 version of C++ (at least).

Note: Code Review related comments will be preceded by "CR: ", and are unnecessary in "real code". The comments without "CR: " however are comments that help future readers and should remain.

#include <memory> // CR: std::unique_ptr<T> is there

class List {
public:
    List();
    void pushBack(int value); // CR: users don't care about "nodes"
                              // CR: but care about position.
    void print() const;   // CR: List::printList is redundant

private:
    // CR: Implementation details (private stuff) go last,
    // CR: because users don't care about them.

    // CR: C++ style declaration
    // CR: Stripped off unnecessary (and unused) members
    struct Node {
        Node(int v);

        int value;
        std::unique_ptr<Node> next;
    };

    // A single member is needed,
    // but as an optimization we keep a pointer to the tail.
    std::unique_ptr<Node> head;
    Node* tail;
};

The unique_ptr is a new C++11 utility, which takes care of correctly calling the destructor of your item (and thus free the memory, in your case).


List::List(): head(), tail() {}

List::Node::Node(int v): value(v), next() {}

It is a good practice to initialize all data members (even those which would not require it), as this means getting into the habit of initializing them and also avoids wondering later "do I need or do I not?" => don't make me think!


void List::pushBack(int value) {
    // Check for empty list
    if (this->tail == nullptr) {
        this->head.reset(new Node(value));
        // C++14 alternative: this->head = std::make_unique<Node>(value);

        this->tail = this->head.get();

        // CR: early return => keep flow linear
        return;
    }

    this->tail->next.reset(new Node(value));
    // C++14 alternative: this->tail->next = std::make_unique<Node>(value);

    this->tail = this->tail->next.get();   
}

It is a good practice to avoid nesting and sprawling code blocks, by returning early we keep the flow linear. Thus, check for edge cases at the start:

  • it makes it easier to check whether they are handled (or not).
  • it makes it easier to read the code: a computer is good at handling context (and restoring the previous context when getting to the end of a block) but your brain isn't.

Note that keeping the flow linear reduces the Cyclomatic Complexity of the function, that is, the number of potential execution paths. The less execution paths possible, the easier it is to understand a function purpose and check its correctness.

Note: C programmers advocate Single Entry Single Exit (SESE) because they have to handle resources (such as allocated memory) manually, and having a single exit make it easier to check that all resources are properly disposed off before exiting (and make it easier not to forget to dispose when adding a resource or an exit). In C++, since resources should NEVER be manually disposed off, SESE is unnecessary, and therefore we instead strive for clarity.


void List::print() const {
    for (Node* current = this->head.get(); current; current = current->next.get()) {
        std::cout << " " << current->value << "\n";
    }
}

The function should be const as it does not modify the List.

A for loop is better than a while loop in that it summarizes all the loop-related stuff in one place (initialization, iteration, exit) making it easier to check all those in one fell swoop... as a corollary, weird loops (those happen too) should be expressed as while loops, immediately alerting the reader that they are special (and deserve special attention).

There is no need to use endl, and indeed it is better not to in general. While its name means end of line, what endl does it actually more subtle:

  • it adds an end of line character
  • it calls .flush() on the stream, ensuring the output is fully transferred (to either disk or screen or ...)

The latter is generally completely unnecessary.

Note: C++ streams are complex beasts, and will actually transform \n to the OS-specific end of line markers, thus \r\n on Windows, automatically.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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