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

• user1118321: 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?
• Frank Doe: 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.

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

• Frank Doe: 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.
• Loki Astari: 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.
• Loki Astari: 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.

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.