3
\$\begingroup\$

This is a follow up to this question. I have taken the suggestions given to me and implemented them in my code, as well as moving towards adding more features to the program. I am unfortunately required to use a linked list for storing contacts, so I cannot adapt to your feedback for that, but thank you all for helping me gain further knowledge of C++.

#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;
        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(int number) const
    {
        int contactNum = number;
        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 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
    std::string position;
    std::string firstName;
    std::string lastName;
    std::string phoneNumber;
    std::string choice;
    int x, converted_position;

    while (1) {
        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 (Coming Soon)" << "\n";
        std::cout << "6. Exit The Program" << "\n\n";
        std::cin >> choice;

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

        switch (x)
        {
        case 1:
            std::cout << "\n";
            Contacts.display_all(); // display all contacts
            std::cout << "\n";
            break;
        case 2:
            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); // create the contact in the linked list
            std::cout << "\n";
            break;
        case 3:
            std::cout << "Enter the contact number of the contact you would like to remove: ";
            std::cin >> position;
            try {
                converted_position = std::stoi(position);
            }
            catch (...) {
                converted_position = 0;
            }
            Contacts.delete_position(converted_position); // delete contact from list
            break;
        case 4:
            std::cout << "Enter the contact number of the contact you would like to edit: ";
            std::cin >> position;
            try {
                converted_position = std::stoi(position);
            }
            catch (...) {
                converted_position = 0;
            }
            Contacts.display_contact(converted_position);
            std::cout << "\nEditing contacts is coming soon." << "\n";
            break;
        case 5:
            std::cout << "\nSearching contacts is coming soon." << "\n";
            break;
        case 6:
            exit(1); // exit the program with exit code 1
            break;
        default:
            std::cout << "\n" << choice << " is not an option. Please select a valid option." << "\n";
            break;
        }
    }
    Contacts.pause();
    return 0;
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

This is easy-to-read code, which is great. It means that if you have to maintain it in the future, it should be relatively easy. That skill will come in handy in the future for you! I see a few things that could be improved.

Avoid Magic Numbers

In main(), where you have the event loop where you get inputs from the user and then act on those inputs, you have a switch statement that has a bunch of numbered cases. It's really hard to remember what 5 or 6 is at the bottom of the list, when the explanatory text is up above. And if you ever change the ordering of the options or insert any new ones, the switch will need to be updated, too. And if you ever have to use those same values anywhere else, then when you update them in one place, you need to update them everywhere.

The way to fix this is to used some form of named constants. C++ allows you to choose from many, but in this case, the most sensible choice is to use an enum for the choices. Something like this:

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

Now if you want to insert a new one between 2 existing values, you can just put it where it belongs and recompile, and all the existing choices will continue to work, even though some of their values have changed. Your switch statement becomes much easier to read, too:

switch (x)
{
    case ShowAll:
        ...code for showing all
    break;
    
    case AddContact:
        ...code for adding a contact
    break;
    ... etc.
}

And because they're named, you no longer have to remember, "Was #4 edit, or quit?"

Simplify

There are some things that are more complicated than they need to be. For example, std::can can directly read an int. You don't need to use stoi() to convert a std::string to an int, just read into an int like this:

std::cin >> x;

This avoids each of the try/catch blocks.

The other place where I see the need for simplification is in the List::delete_position() method. It looks to me like there are just too many special cases in that code. Perhaps we can write it more simply?

void delete_position(int pos)
{
    if (pos < 1 || head == nullptr)
    {
        return;
    }
    
    // Get the node to delete and the node that came before it
    node* victim { head };
    node* prev { nullptr };
    int nodePos = 1;
    while (nodePos < pos)
    {
        prev = victim;
        victim = victim->next;
        nodePos++;
    }
    
    if (victim != nil)
    {
        // Handle any special cases that need to be handled
        if (victim == head)
        {
            head = head->next;
        }
        
        if (victim == tail)
        {
            tail = prev;
            tail->next = nullptr;
        }
        
        // Delete the node
        delete victim;
    }
}

This gives us a single loop to find the correct node, and puts all the special cases into a couple of ifs.

Don't Use Infinite Loops

In your main() function you've chosen to use an infinite loop when there's a clear condition that allows you to stop the loop. It seems kind of odd to make a loop infinite, only to put a condition inside it that says how to get out of the loop. Just put the condition directly in the while statement:

bool exit_contacts = false;
while (!exit_contacts)
{
    ... code from the original loop
    switch (x)
    {
        ...
        case ExitContacts:
            exit_contacts = true;
        break;
}

Now I don't have to search the body of the loop to see if there's an extra out (or multiple of them). I can see that when exit_contacts is true, we're done.

Error

Your display_contact() method is almost identical to display_all(), except that you're not incrementing the contactNum. I think this is a mistake and that it's supposed to first find the contact with the passed-in number, and then print it. But maybe I've misunderstood?

Use More Functions

I think this code would benefit from breaking some things out into separate functions. In your main() function, I'd break out the directions into its own function named something like display_instructions(). Given the amount of code you have in each case of the switch statement, I'd probably break each one of those out into its own function, as well. It would be fine to leave case 6 and the default case, since they're so short. (I assume case 5 will get longer eventually?)

In your display_all() and display_contact() function you have some repeated code. You might consider making a display_contact(node* contact) method that just displays a single contact that's passed in as an argument. Then your display_all() method could call it for each node, and your display_contact(int number) method could find the appropriate node and call display_contact(node* contact) to actually display it.

Overall, this is pretty good and the above suggestions are mostly cosmetic.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your feedback! For my display_contact() method, you are correct, it is supposed to find the contact with the number given, and then print it. I wrote this for editing a contact, and also for searching for it. However I'm not sure how I will go about searching contact names, etc, in a linked list. So that will be my next challenge. \$\endgroup\$
    – Frank Doe
    Dec 5, 2017 at 14:24

Your Answer

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

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