Phone Book Program in C++ - Final Follow Up

Question

I hope I am not going against any rules on this site. This is my last follow up question, and I believe I am ready to turn this code in. (It is a homework assignment and I am required to use linked lists) This code is a follow up to this question. I certainly have gained knowledge from this community and am very appreciative of your feedback.

Some things I have implemented since the first iteration:

Implemented enum for a menu instead of using magic numbers, No longer using using namespace std, No longer using system("pause"), No longer using std::endl but now using \n, Fixed memory leaks

I still neeed to implement an undo/redo stack and fix my edit function because it is not working correctly.

#include "stdafx.h"
#include <iostream>
#include <string>

class List
{
private:
    struct node
    {
        std::string firstName;
        std::string lastName;
        std::string phoneNumber;
        node *next;
    };
    node *head, *tail;

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

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

    void display_all() const
    {
        int contactNum = 0;
        if (head == NULL) {
            std::cout << "You have no contacts.";
        }
        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";
            std::cout << "Phone Number: " << curr->phoneNumber << "\n";
        }
    }

    void display_contact(std::string first, std::string last)
    {
        bool found;

        node *curr = head;
        found = false;
        int idx = -1;

        while (curr != NULL & !found)
        {
            if (curr->firstName == first && curr->lastName == last)
            {
                idx++;
                found = true;
            }
            else
            {
                curr = curr->next;
            }

        }
        if (found)
        {
            std::cout << "First Name: " << curr->firstName << "\n";
            std::cout << "Last Name: " << curr->lastName << "\n";
            std::cout << "Phone Number: " << curr->phoneNumber << "\n";
            delete_position(idx);
        }
        else
        {
            std::cout << "\n" << first << " " << last << " was not found.\n";
        }
    }

    void name_search(std::string titleSearched)
    {
        bool found;

        node *curr = head;
        found = false;

        while (curr != NULL & !found)
        {
            if (curr->firstName == titleSearched)
            {
                found = true;
            }
            else
            {
                curr = curr->next;
            }

        }
        if (found)
        {
            std::cout << "\n" << titleSearched << " was found.\n";
            std::cout << "-----------------------------------\n";
            std::cout << "First Name: " << curr->firstName << "\n";
            std::cout << "Last Name: " << curr->lastName << "\n";
            std::cout << "Phone Number: " << curr->phoneNumber << "\n";
        }
        else
        {
            std::cout << "\n" << titleSearched << " was not found.\n";
        }
    }


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

    void pause() {
        getchar();
    }

};

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

    enum {
        ShowAll = 1,
        AddContact,
        RemoveContact,
        EditContact,
        SearchContacts,
        ExitContacts
    };

    bool exit_contacts = false;

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

        try {
            x = std::stoi(choice);
        }
        catch (...) {
            x = 0;
        }

        switch (x)
        {
        case ShowAll:
            std::cout << "\n";
            Contacts.display_all();
            std::cout << "\n";
            break;
        case AddContact:
            std::cout << "\nEnter the contacts' first name: ";
            std::cin >> firstName;
            std::cout << "\nEnter the contacts' last name: ";
            std::cin >> lastName;
            std::cout << "\nEnter the contacts' phone number: ";
            std::cin >> phoneNumber;
            Contacts.create_contact(firstName, lastName, phoneNumber);
            std::cout << "\n";
            break;
        case RemoveContact:
            std::cout << "\nEnter the contact number of the contact you would like to remove: ";
            std::cin >> position;
            Contacts.delete_position(position);
            break;
        case EditContact:
            std::cout << "\nTo find the contact you would like to edit: ";
            std::cout << "\nEnter the contacts' first name: ";
            std::cin >> firstName;
            std::cout << "\nEnter the contacts' last name: ";
            std::cin >> lastName;
            Contacts.display_contact(firstName, lastName);
            std::cout << "\nEnter the contacts' NEW first name: ";
            std::cin >> firstName;
            std::cout << "\nEnter the contacts' NEW last name: ";
            std::cin >> lastName;
            std::cout << "\nEnter the contacts' NEW phone number: ";
            std::cin >> phoneNumber;
            Contacts.create_contact(firstName, lastName, phoneNumber);
            break;
        case SearchContacts:
            std::cout << "\nEnter the contacts' first name: ";
            std::cin >> firstName;
            Contacts.name_search(firstName);
            break;
        case ExitContacts:
            exit_contacts = true;
            break;
        default:
            std::cout << "\n" << choice << " is not an option. Please select a valid option." << "\n";
            break;
        }
    }
    Contacts.pause();
    return 0;
}

Answer 1

On a first read looks good.
Would use named functions in a few places to increase readability.

Some other minor things:

It is more traditional for user types to have an initial uppercase letter.

 struct node

 // prefer
 struct Node

This makes it easier to spot the difference between types and objects.

 Node("Loki", "Astari", "[email protected]");
 // Easy to spot that is a type beign created.
 node("Loki", "Astari", "[email protected]");
 // Is that a node being created or a function call?
 // Harder to tell not impossible. But if you use the above nameing
 // convention functions and objects are easy to spot in the code and
 // Types are easy to spot when they are being used. 

If you are using C++03 then NULL is still acceptable. But nearly all implementations now accept nullptr. This is better it is type-safe.

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

When passing values here:

    void create_contact(std::string first, std::string last, std::string cellNumber)

You are passing the parameters by value. This means a copy is passed to the function. It is more normal to pass values by const reference to avoid the copy.

    void create_contact(std::string const& first, std::string const& last, std::string const& cellNumber)

You should simplify your code with a constructor (or use the new list initializer).

        node *temp = new node;
        temp->firstName = first;
        temp->lastName = last;
        temp->phoneNumber = cellNumber;
        temp->next = NULL;

Easier to write as:

        Node* node = new Node(first, last, cellNumber, nullptr);

Or you can use the list initialization if you don't want to write a constructor:

        Node* node = new Node{first, last, cellNumber, nullptr};

Building a list. An easy way to have a list is to have a fake node in the list (this is called a sentinel). The sentinel marks the beginning (or the beginning and end in a circular list). It does not contain data and you don't dynamically initialize it. The advantage is that you don't need to test for nullptr as there is always a member of the list so adding and removing (as long as you don't try and remove the sentinel) become much easier to write.

        if (head == NULL)
        {
            head = temp;
            tail = temp;
            temp = NULL;
        }
        else
        {
            tail->next = temp;
            tail = temp;
        }

Now is simply:

        tail->next = temp;
        tail       = temp;

Your declarations in your class become:

        Node      head;   // Always have a fake head node.
        Node*     tail;   // tail always points at the last.
                          // Its empty if tail points at head.

This works fine:

    void display_all() const

But std::cout is not the only stream you can print to. How about a file or a socket or an internal buffer before writting to a socket.

I would pass the stream you want to display on as a parameter. It can alwjays default to std::cout but allow a user to specify an alterantive:

    void display_all(std::ostream& outStream = std::cout) const

Sure this is fine:

        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";
            std::cout << "Phone Number: " << curr->phoneNumber << "\n";
        }

But why not ask the node to stream itself.

        for (node *curr = head; curr; curr = curr->next) {
            outStream << *curr;
        }

Each function should have one action:

    void display_contact(std::string first, std::string last)

This function has two distinct actions. 1) Find a Node 2) display a node. You can separate these out into there individual parts so they can be re-used more easily.

    void display_contact(std::ostream const& ooutStream, std::string const& first, std::string const& last) {
        auto data = list.find(first, last);
        if (data !=list.end()) {
            outStream << data;
        }
        else {
            std::cout << "No person found called: " << last << " " << first << "\n";
        }

O look here: We have found a re-use case for print out a Node.

        if (found)
        {
            std::cout << "First Name: " << curr->firstName << "\n";
            std::cout << "Last Name: " << curr->lastName << "\n";
            std::cout << "Phone Number: " << curr->phoneNumber << "\n";
            delete_position(idx);
        }

A lot of the user interface code could be spint into functions.

    case AddContact:
        std::cout << "\nEnter the contacts' first name: ";
        std::cin >> firstName;
        std::cout << "\nEnter the contacts' last name: ";
        std::cin >> lastName;
        std::cout << "\nEnter the contacts' phone number: ";
        std::cin >> phoneNumber;
        Contacts.create_contact(firstName, lastName, phoneNumber);
        std::cout << "\n";
        break;

Easier to read as:

    case AddContact:
        addContact(contacts);
        break;



void addContacts(Contacts& contacts)
{
        std::cout << "\nEnter the contacts' first name: ";
        std::cin >> firstName;
        std::cout << "\nEnter the contacts' last name: ";
        std::cin >> lastName;
        std::cout << "\nEnter the contacts' phone number: ";
        std::cin >> phoneNumber;
        contacts.create_contact(firstName, lastName, phoneNumber);
        std::cout << "\n";
}

Answer 2

At the first glance, Program looks perfectly fine. But there is a Logical Error. When You ask user to input the choice, you took the choice in a string. Ever wondered what happens when user types in 1 2 3 4 and then presses enter. The exception won't handle it and therefore would just call switch for each of those cases.