Phone Book Program in C++

Question

I am starting out a school project to build a phone book program in C++. It stores contacts in a linked list. This currently does not have all the validations I would like to implement, and is all in one phonebook.cpp file. I am required to implement a linked list as part of the assignment. Any constructive feedback would be appreciated.

#include "stdafx.h"
#include <iostream>
#include <string>
using namespace std;

struct node
{
    string firstName;
    string lastName;
    node *next;
};

class List
{
private:
    node *head, *tail;
public:
    List()
    {
        head = NULL;
        tail = NULL;
    }

    void create_contact(string first, string last) // Create node and add it onto the linked list
    {
        node *temp = new node;
        temp->firstName = first;
        temp->lastName = last;
        temp->next = NULL;
        if (head == NULL)
        {
            head = temp;
            tail = temp;
            temp = NULL;
        }
        else
        {
            tail->next = temp;
            tail = temp;
        }
    }

    void display_all() // Prints out current trip
    {
        int contactNum = 1;
        node *temp = new node;
        temp = head;
        while (temp != NULL)    // Loop through the list while the temporary node is not empty
        {
            cout << "\nContact Number: " << contactNum << endl;
            cout << "First Name: " << temp->firstName << endl;
            cout << "Last Name: " << temp->lastName << endl;
            ++contactNum;
            temp = temp->next;
        }
    }

    void delete_position(int pos)   // delete a stop by using the position in the list
    {
        node *current = new node;
        node *previous = new node;
        node *next = new node;
        current = head;
        for (int i = 1;i<pos;i++)   // Loop through the link list while the current node is not empty
        {
            if (current == NULL)
                return;
            previous = current;
            current = current->next;
        }
        next = current->next;
        previous->next = current->next;
        delete current;
    }

    void delete_head()  // delete head node
    {
        node *temp = new node;
        temp = head;
        head = head->next;
        delete temp;
    }
};

int main()
{
    List Contacts; // create a Contacts item for the List class
    int choice, position;
    string firstName;
    string lastName;

    while (1) {
        cout << "\nWhat would you like to do?: " << endl;
        cout << "1. Show All Contacts" << endl;
        cout << "2. Add A Contact" << endl;
        cout << "3. Remove A Contact" << endl;
        cout << "4. Search Contacts (Coming Soon)" << endl;
        cout << "5. Exit The Program" << endl;
        cin >> choice;

        switch (choice)
        {
        case 1:
            cout << endl;
            Contacts.display_all(); // display all contacts
            cout << endl;
            break;
        case 2:
            cout << "\nEnter your first name: ";
            cin >> firstName;
            cout << "\nEnter your last name: ";
            cin >> lastName;
            Contacts.create_contact(firstName, lastName); // create the contact in the linked list
            cout << endl;
            break;
        case 3:
            cout << "Enter the contact number of the contact you would like to remove: ";
            cin >> position;
            if (position == 1)
                Contacts.delete_head();
            else
                Contacts.delete_position(position); // delete contact from list
            break;
        case 5:
            exit(1);
            break;
        default:
            cout << "\n" << choice << " is not an option. Please select a valid option." << endl;
            break;
        }
    }
    system("pause");
    return 0;
}

Answer 1

There are a number of things that could be improved, and I hope you find this list of suggestions useful.

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 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();
}

General portability

This code could be made portable if, in addition to the changes in the previous point, you omit the Windows-only include files #include "stdafx.h".

Name things consistently

We have node and then List, with an uppercase initial letter. It's usually better to name things consistently so that readers of the code (including you!) will have an easier time understanding it.

Use nullptr rather than NULL

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

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.

Don't leak memory

In the delete_position code, we have this line:

node *next = new node;

However, that next is never deleted and is assigned but not otherwise usefully used in the program. The code for this should be refactored.

Understand pointers

In several cases the code does something like this:

node *temp = new node;

However, new allocates memory and actually creates a new node. What's probably actually needed in most cases is just a pointer. For example, the display_all function could be written instead like this:

void display_all() const 
{
    int contactNum = 0;
    for (node *curr = head; curr; curr = curr->next) 
    {
        std::cout << "\nContact Number: " << ++contactNum << "\n";
        std::cout << "First Name: " << curr->firstName << "\n";
        std::cout << "Last Name: " << curr->lastName << "\n";
    }
}

Use const where practical

Functions such as display_all which do not alter the underlying List should be declared const. In general, whenever you are writing a variable or function prototype look for places you can use const.

Prefer modern initializers for constructors

The code currently has this constructor:

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

I'd advocate using the more modern C++11 style:

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

Perform better error checking for user input

As it is currently written, if the user types "Norway" as the choice the code will loop endlessly. Better would be to input a string and then use std::stoi to convert it to a number.

Keep private data private

Outside the List, there is no need to have a node, so I would recommend that the node definition should be a private definition within List.

Keep implementation details private

Don't require the user of the List class to call delete_head or delete_position depending on which node it is. Instead, have a single delete_position the does the right thing for either.

Simplify deletion of nodes

Here's an alternative method for deleting a node that doesn't leak memory and incorporates the previous suggestion.

void delete_position(int pos)    
{
    if (pos < 1 || head == nullptr) 
        return;
    node *victim{head};
    if (pos == 1) {  // deleting head
        if (tail == victim) {
            head = tail = nullptr;
        } else {
            head = head->next;
        }
    } else { 
        // deleting non-head node
        node *prev{head};
        for (pos -= 2; pos; --pos) {
            if (prev == nullptr)
                return;
            prev = prev->next;
        }
        victim = prev->next;
        prev->next = victim->next;
        if (tail == victim) {
            tail = prev;
        }
    }
    delete victim;
}

Provide a destructor

Because the class allocates memory, it should include a destructor to free any memory that might have been allocated.

Answer 2

Avoid using namespace std

This can cause name collisions because it adds every name in the std namespace to the global namespace. For a small program like this one it's unlikely that you'll run into any problems (then again, maybe not) but it's best to get into the habit of using the std:: prefix on names in the std namespace.

Alternatively, you can introduce using declarations like using std::cout; to add specific names to the global namespace.

Avoid std::endl in favor of \n

std::endl flushes the stream, which can cause a loss in performance.

Don't reinvent the wheel

Since you've tagged this homework you may be required to implement a linked list as part of the assignment. If not, however, you can use std::list with your node struct or something similar as the data type for the std::list (you'd be able to remove the node *next field of the node struct).

Here's a basic demo of what such a contacts list would look like:

#include <iostream>
#include <string>
#include <list>

struct Name {
    std::string first;
    std::string last;
};

class Contacts {
private:
    std::list<Name> contacts;
public:
    void add(Name name) {
        contacts.push_back(name);
    }

    void add(std::string first, std::string last) {
        contacts.push_back(Name{first, last});
    }

    void display() {
        for (auto name : contacts) {
            std::cout << name.first << " " << name.last << '\n';
        }
    }
};

int main() {
    Contacts contacts;
    contacts.add("First", "Last");
    Name name;
    name.first = "Foo";
    name.last = "Bar";

    contacts.add(name);
    contacts.display();
    return 0;
}

Your comments could use some improvement

Your code doesn't contain many comments, and it would be helpful to see more of them. For example, it's not immediately obvious why there's a separate function to delete the head (List::delete_head()) vs. a different position (List::delete_position()). You should add comments to explain the requirements of the different functions. Even if you know why you have two different functions today, you might not remember if you look at the code again in a month.

Some of the comments you do have aren't very useful. For example:

while (temp != NULL)    // Loop through the list while the temporary node is not empty

That comment doesn't tell me anything that the actual while (temp != NULL) code hadn't already told me more succinctly.