2
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • \$\begingroup\$ It seems really surprising to me that your display_contact() method deletes the object it displays. Are you sure you want to do it that way? \$\endgroup\$ Dec 12 '17 at 5:39
  • \$\begingroup\$ Since it is part of the edit function, I intended for it to work this way. So a user will think they are editing the contact but in reality just overwriting the data that is there. \$\endgroup\$
    – Frank Doe
    Dec 12 '17 at 12:58
3
\$\begingroup\$

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", "Loki.Astari@gmail.com");
 // Easy to spot that is a type beign created.
 node("Loki", "Astari", "Loki.Astari@gmail.com");
 // 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";
}
\$\endgroup\$
3
  • \$\begingroup\$ Thank you for your feedback! It is part of the assignment to not use a file for output, and display everything in the console. I'm sure using a constructor would be easier for initialization, but I'm not sure how to write one. \$\endgroup\$
    – Frank Doe
    Dec 12 '17 at 13:04
  • \$\begingroup\$ I did not say use a file for output. What I said was make the code generic enough so that it can be used by any stream type. \$\endgroup\$ Dec 12 '17 at 19:40
  • \$\begingroup\$ You wrote a constructor for List! A constructor is just like any other function. The only difference is that it is named after the class (That is a good starting point). BUT you don't need a constructor use the brace initializer for Node. \$\endgroup\$ Dec 12 '17 at 19:43
2
\$\begingroup\$

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.

\$\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.