C++11 Command Line Address Book

Question

A friend is doing a CS course that has a command line address book as a term assignment. Although I am a programmer in industry, I don't do a lot of C++, so I ended up doing this as a learning exercise for C++. The address book is supposed to read a CSV file named addressBook.csv and parse contacts in the specified order (first,middle,last,phone,address) and enable sorting, searching, and writing to disk. There are "known unknowns" and "unknown unknowns". First of all, I want to hear about style issues that I don't even know enough to ask about. But I did have some specific questions that came up.

1. line 100: I have to friend ContactListManager so it can dig at the underlying vector in order to find max column widths. It seems dirty. I should be able to expose a contacts iterator without letting ContactListManager know that it's really just a std::vector. How would you expose that? Would you bother to hide the fact that ContactList is a glorified std::vector<Contact> in real code, or are these classes so close together that you wouldn't bother?

2. 158-162: Any better way to express going through all of these named members? It's an awful lot of code duplication.

3. size_t. My understanding is that size_t is just a typedef that can hold any index to an array without overflow. Does this apply to std::vector as well?

4. Specifically I am getting compiler warnings losing integer precision when I pass size_t to setw. What would you do about that? Is there a way to get rid of them by maxing them. What I really want to is to setw(min(my_size_t, 80)). Maybe I can do (unsigned int)min(my_size_t, (size_t)80). Is there a better way to cast to express the concept?

5. 446: Everything is done in memory with an explicit commit step. The dirty flag prompts the user if they try to quit with unsaved changes. But if they drop an EOF onto stdin, I don't know how to recover the stream. in.ignore() + in.clear() don't recover the stream. I can't find anything on the internet that works in my terminal to this. Is this just really really hard to get around?

6. 127: contacts.push_back(c). I fear that I'm making a copy of c and pushing that onto the vector, when in fact this local variable is about to go out of scope. Because I need to check parsefail before I push, I can't emplace_back. Can I std::move?

Code:

//
//  main.cpp
//  AddressBook
//
//  Created by Mason Kramer on 11/15/16.
//  Copyright © 2016 Mason. All rights reserved.
//

#include <iostream>
#include <fstream>
#include <string>
#include <sstream>
#include <vector>
#include <iomanip> /* setw */
#include <algorithm>    /* random_shuffle */

const bool QUIET = false;
const bool VERBOSE = false;

/* always separate side effects like error logging so that you can handle them differently later */
void error(std::string msg) {
    if (!QUIET) {
        std::cerr << msg << std::endl;
    }
}
void vocalize(std::string msg) {
    if (VERBOSE) {
        std::cerr << msg << std::endl;
    }
}

/* trim classes cribbed from SO */
static inline std::string &ltrim(std::string &s) {
    s.erase(s.begin(), std::find_if(s.begin(), s.end(),
                                    std::not1(std::ptr_fun<int, int>(std::isspace))));
    return s;
}

// trim from end
static inline std::string &rtrim(std::string &s) {
    s.erase(std::find_if(s.rbegin(), s.rend(),
                         std::not1(std::ptr_fun<int, int>(std::isspace))).base(), s.end());
    return s;
}

// trim from both ends
static inline std::string &trim(std::string &s) {
    return ltrim(rtrim(s));
}

class Contact {
    char field_separator = ',';
    void get_optional_field(std::istream& in, std::string& field, const std::string& fieldid) {
        std::getline(in, field, field_separator);
        if (in.fail()) {
            parse_fail = true;
        }
        field = trim(field);
    }
    void get_field(std::istream& in, std::string& field, const std::string& fieldid) {
        get_optional_field(in, field, fieldid);
        if (field == "") {
            parse_fail = true;
            vocalize("Field " + fieldid + " was empty");
        }
    }

    public:
    bool parse_fail = false;
    Contact(std::string first, std::string middle, std::string last, std::string phone, std::string address):
        first_name{first},
        middle_name{middle},
        last_name{last},
        phone_number{phone},
        address{address} {}

    Contact(std::string line) { /* parse a single line for a contact, set failure if parse failed */
        std::stringstream ss {line};
        get_field(ss, first_name, "first name (1)");
        get_optional_field(ss, middle_name, "middle name (2)");
        get_field(ss, last_name, "last name (3)");
        get_field(ss, phone_number, "phone number (4)");
        get_field(ss, address, "address (5)");
    };
    std::string  first_name;
    std::string  middle_name;
    std::string  last_name;
    std::string  phone_number;
    std::string  address;

    std::string serialize() const {
        std::stringstream ss;
        ss << first_name << field_separator << middle_name << field_separator << last_name << field_separator << phone_number << field_separator << address;
        return ss.str();
    }
};

/* all the things you can do to a list of contacts */
class ContactList {
    friend class ContactListManager;
    std::vector<Contact> contacts;

    std::string normalize(std::string str)
    {
        // convert the string to upper case
        for (std::string::size_type i = 0; i < str.length(); ++i)
        {
            str[i] =  std::tolower(str[i]);
        }

        return trim(str);
    }
public:
    ContactList(): contacts{std::vector<Contact> {}} {}
    ContactList(std::ifstream& in) {
        std::string line;
        int line_number = 1;

        if (in.good()) {
            while (getline(in, line)) {
                Contact c {line};
                /* We don't have to completely fail if one line is weird */
                if (c.parse_fail) {
                    error("Failed to parse a contact on line number " + std::to_string(line_number));
                }
                else {
                    contacts.push_back(c);
                }
                ++line_number;
            }
        }
    }
    size_t count() {
        return contacts.size();
    }
    void shuffle() {
        std::random_shuffle(contacts.begin(), contacts.end());
    }

    void remove_record(unsigned long idx) {
        contacts.erase(contacts.begin() + idx);
    }

    void add_record(Contact c) {
        contacts.push_back(c);
    }

    void sort_last_name() {
        std::sort(contacts.begin(), contacts.end(), [](Contact &a, Contact &b) {
            return a.last_name < b.last_name;
        });
    }
    std::vector<size_t> search(std::string t) {
        std::vector<size_t> matches;
        std::string term { normalize(t) };
        for (size_t i = 0; i < contacts.size(); i++) {
            Contact &c = contacts[i];
            if (normalize(c.last_name).find(term) != std::string::npos) { matches.push_back(i); continue; }
            if (normalize(c.first_name).find(term) != std::string::npos) { matches.push_back(i); continue; }
            if (normalize(c.middle_name).find(term) != std::string::npos) { matches.push_back(i); continue; }
            if (normalize(c.phone_number).find(term) != std::string::npos) { matches.push_back(i); continue; }
            if (normalize(c.address).find(term) != std::string::npos) { matches.push_back(i); continue; }
        }
        return matches;
    }

};

/* 
Something that pairs a ContactList with a file that acts as a persistent database of that contact list,
 and manages reads and writes to the backing file.
 */

class ContactListManager {
    bool dirty = false;
    ContactList list {};
    const char* databaseFile;
public:
    bool parsefailure = false;
    size_t count() {
        return list.count();
    }
    ContactListManager(const char* databaseFile): databaseFile{databaseFile} {
        std::ifstream in;
        in.open(databaseFile);

        if (in.good()) {
            list = ContactList{in};
        }
        else {
            parsefailure = true;
            if (in.eof()) {
                error("Database file was empty");
            }
            else {
                error("Unable to open database file for parsing");
            }
        }

    }

    bool safe_commit() {
        if (dirty) {
            std::string tmpnam { "buffer.db" };
            std::ofstream in;
            in.open(tmpnam.c_str());
            in << *this;
            in.close();

            /* Write the new database to a temp file. Then try to parse that file back into a ContactList.
             If that succeeds then we have a valid database file, so replace the current database file with the new one */
            ContactListManager test {tmpnam.c_str()};
            if (!test.parsefailure && test.count() == count()) {
                std::remove(databaseFile);
                std::rename(tmpnam.c_str(), databaseFile);
                dirty = false;
                return true;
            }
            else {
                error("Commit would create an unparseable database, aborting.");
                return false;
            }
        }
        return true;
    }

    void sort_last_name() {
        dirty = true;
        list.sort_last_name();
    }
    void shuffle() {
        list.shuffle();
    }

    void remove_record(unsigned long idx) {
        list.remove_record(idx);
    }

    void add_record(Contact c) {
        list.add_record(c);
    }

    friend std::ostream& operator<< (std::ostream& stream, const ContactListManager& csm) {
        for (auto &c : csm.list.contacts) {
            stream << c.serialize() << std::endl;
        }
        return stream;
    }

    size_t max_width( std::string(*field)(const Contact&) ) const {
        size_t max = 0;
        for (auto& c : list.contacts) {
            auto str = field(c);
            max = std::max(str.length(), max);
        }
        return max;
    }

    size_t max_width_idx(std::vector<size_t> idxes, std::string(*field)(const Contact&) ) const {
        size_t max = 0;
        for (auto& idx : idxes) {
            auto str = field(list.contacts[idx]);
            max = std::max(str.length(), max);
        }
        return max;
    }

    /* Print these specific indexes */
    void pretty_print(std::ostream &stream, std::vector<size_t> idxs) const {
        if (idxs.size() == 0) {
            return;
        }

        /* find the max length of each field so I can pretty each column with a fixed width */
        size_t last_w = max_width_idx(idxs, [](const Contact &c) { return c.last_name; });
        size_t first_w = max_width_idx(idxs, [](const Contact &c) { return c.first_name; });
        size_t middle_w = max_width_idx(idxs, [](const Contact &c) { return c.middle_name; });
        size_t phone_w = max_width_idx(idxs, [](const Contact &c) { return c.phone_number; });
        size_t address_w = max_width_idx(idxs, [](const Contact &c) { return c.address; });

        last_w = std::max(last_w, (size_t)5) + 2;
        first_w = std::max(first_w, (size_t)5) + 2;
        middle_w = std::max(middle_w, (size_t)6);
        address_w = std::max(address_w, (size_t)7);
        phone_w = std::max(phone_w, (size_t)5);

        std::string spacer((int)last_w + first_w + middle_w + address_w + phone_w + 15, '-');

        stream << spacer << std::endl;

        stream << std::left <<
        std::setw(7)             << "Index"   << std::setw(0) << ""   <<
        std::setw(last_w)        << "Last"    << std::setw(0) << ""   <<
        std::setw(first_w)       << "First"   << std::setw(0) << ""    <<
        std::setw(middle_w)      << "Middle"  << std::setw(0) << " | "  <<
        std::setw(address_w)     << "Address" << std::setw(0) << " | "  <<
        std::setw(phone_w)       << "Phone"   << std::setw(0) << " |"  << std::endl;

        stream << spacer << std::endl;

        for (auto& idx : idxs) {
            auto c = list.contacts[idx];
            stream <<
            std::setw(7)         << idx + 1          << std::setw(0) << ""   <<
            std::setw(last_w)    << c.last_name  + ", "  << std::setw(0)   <<
            std::setw(first_w)   << c.first_name   << std::setw(0) << ""    <<
            std::setw(middle_w)  << c.middle_name  << std::setw(0) << " | "  <<
            std::setw(address_w) << c.address      << std::setw(0) << " | "  <<
            std::setw(phone_w)   << c.phone_number << std::setw(0) << " |"  << std::endl;
        }
    }
    /* Print some or all contacts. If begin is given, start from the beginth record. If n is greater than 0, print up to n records, otherwise print to the end */
    void pretty_print(std::ostream &stream, size_t begin = 0, size_t n = 0) const {
        /* find the max length of each field so I can pretty each column with a fixed width */
        size_t last_w = max_width([](const Contact &c) { return c.last_name; });
        size_t first_w = max_width([](const Contact &c) { return c.first_name; });
        size_t middle_w = max_width([](const Contact &c) { return c.middle_name; });
        size_t phone_w = max_width([](const Contact &c) { return c.phone_number; });
        size_t address_w = max_width([](const Contact &c) { return c.address; });

        last_w = std::max(last_w, (size_t)5) + 2;
        first_w = std::max(first_w, (size_t)5) + 2;
        middle_w = std::max(middle_w, (size_t)6);
        address_w = std::max(address_w, (size_t)7);
        phone_w = std::max(phone_w, (size_t)5);

        std::string spacer((int)last_w + first_w + middle_w + address_w + phone_w + 15, '-');

        stream << spacer << std::endl;

        stream << std::left <<
        std::setw(7)             << "Index"   << std::setw(0) << ""   <<
        std::setw(last_w)        << "Last"    << std::setw(0) << ""   <<
        std::setw(first_w)       << "First"   << std::setw(0) << ""    <<
        std::setw(middle_w)      << "Middle"  << std::setw(0) << " | "  <<
        std::setw(address_w)     << "Address" << std::setw(0) << " | "  <<
        std::setw(phone_w)       << "Phone"   << std::setw(0) << " |"  << std::endl;

        stream << spacer << std::endl;

        size_t end = list.contacts.size();
        if (n > 0) {
            end = std::min(end, begin + n);
        }

        for (size_t idx = begin; idx < end; idx++) {
            auto c = list.contacts[idx];
            stream <<
            std::setw(7)         << idx + 1          << std::setw(0) << ""   <<
            std::setw(last_w)    << c.last_name  + ", "  << std::setw(0)   <<
            std::setw(first_w)   << c.first_name   << std::setw(0) << ""    <<
            std::setw(middle_w)  << c.middle_name  << std::setw(0) << " | "  <<
            std::setw(address_w) << c.address      << std::setw(0) << " | "  <<
            std::setw(phone_w)   << c.phone_number << std::setw(0) << " |"  << std::endl;
        }
    }
    Contact get_contact_interactive(std::istream& in, std::ostream& out, std::ostream& err) {
        std::string  buff, first, middle, last, phone, address;
        out << "First: ";
        getline(in, buff);
        first = trim(buff);
        out << "Middle: ";
        getline(in, buff);
        middle = trim(buff);
        out << "Last: ";
        getline(in, buff);
        last = trim(buff);
        out << "Address: ";
        getline(in, buff);
        address = trim(buff);
        out << "Phone: ";
        getline(in, buff);
        phone = trim(buff);

        return Contact {first, middle, last, phone, address};
    }

    void usage(std::ostream &out) {
        out << std::endl << "[p]rint [start idx] [num records], [s]ort, [q]uit, [h]elp, [x] shuffle, [f]ind <search string>" << std::endl;
        out << "[a]dd, [d]elete <n>th record, [c]ommit to file" << std::endl;
    }
    void interact(std::istream& in, std::ostream& out, std::ostream& err) {
        std::string command_buff;
        std::string command;
        usage(out);

        while (getline(in, command_buff))
        {
            std::string tok;
            std::stringstream ss{command_buff};

            ss >> tok;
            command = tolower(tok[0]);

            if (command == "q") {
                break;
            }
            if (command == "p") {
                size_t begin;
                size_t n;
                if (ss >> begin) {
                    if (ss >> n) {
                        pretty_print(out, begin-1, n);
                    }
                    else {
                        pretty_print(out, begin-1, 1);
                    }
                }
                else {
                    pretty_print(out);
                }
            }
            if (command == "x") {
                shuffle();
                out << "Shuffled." << std::endl;
            }
            if (command == "s") {
                sort_last_name();
                out << "Sorted." << std::endl;
            }
            if (command == "h") {
                usage(out);
            }
            if (command == "f") {
                std::string rest;
                ss >> rest;
                auto matches = list.search(rest);
                pretty_print(out, matches);
            }
            if (command == "c") {
                safe_commit();
                out << "Committed." << std::endl;
            }
            if (command == "a") {
                add_record(get_contact_interactive(in, out, err));
                out << "Added." << std::endl;
            }
            if (command == "d") {
                int n;
                ss >> n;
                remove_record(n - 1);
                out << "Removed " << n << "." << std::endl;
            }
        }

        if (dirty) {
            out << "You have uncommitted changes, would you like to save them? y/N" << std::endl;

            getline(in, command_buff); /* linux convention is that a simple "enter" at this point is N */
            command = tolower(trim(command_buff)[0]);
            if (command == "y") {
                safe_commit();
                out << "Commited. Goodbye." << std::endl;
            }
            else {
                out << "Will not commit. Goodbye." << std::endl;
            }
        }
    }
};

int main(int argc, const char * argv[]) {
    std::string inFileName {"addressBook.csv"};
    if (argc >= 2) {
        inFileName = std::string{argv[1]};
    }
    std::cout << "Welcome to AddressBook. Managing database file " << inFileName << std::endl;
    ContactListManager csm {inFileName.c_str()};
    if (!csm.parsefailure) {
        std::cout << "I parsed " << csm.count() << " records." << std::endl;
        csm.interact(std::cin, std::cout, std::cerr);
    }
    else {
        std::ofstream fh;
        fh.open(inFileName);
        if (fh.good()) {
            std::cout << "Parsing failed, but you can still interactively add contacts and commit " << std::endl;
            csm.interact(std::cin, std::cout, std::cerr);
        }
        else {
            std::cout << "Parsing failed and I can't write to the designated file location. Exiting." << std::endl;
        }
        fh.close();
    }

    return 0;
}

Answer 1

Here are some things that may help you improve your code.

Fix the bugs

The code currently contains this function:

friend std::ostream& operator<< (std::ostream& stream, const ContactListManager& csm) {
    for (auto &c : csm.list.contacts) {
        stream << c.serialize() << std::endl;
    }
    return stream;
}

However, this won't compile on my machine because contacts is a private member of the ContactList class. This function is a friend of ContactListManager and ContactListManager is a friend of ContactList but in C++ as in life, friendship is not transitive. That is, a friend of your friend isn't necessarily your friend too. This can be addressed (if you'll pardon the pun) by simply creating a similar ostream operator<< function for the ContactList class and using that.

Another bug is that because only sorting by last name sets the dirty flag, if one adds or deletes several records, they won't be committed to the file even if explicitly instructed by the user to commit.

Yet another bug is that remove_record is not range checked, so if we attempt to delete record 7 from an empty database, the program segfaults and crashes.

Use the standard form of main()

The current main declaration looks like this:

int main(int argc, const char * argv[]) {

However, the use of const there may bring portability problems because the only explicitly supported signatures for main are these:

int main()
int main(int argc, char *argv[])
int main(int argc, char **argv)

It might work with your particular compiler, but it's not guaranteed on all compilers because it's not standard. See this question and the related one for details and history.

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.

Reconsider your class design

The ContactListManager has a number of functions that simply "pass through" to the ContactList class, such as this one:

size_t count() {
    return list.count();
}

This strongly suggests that it would probably be better to use inheritance instead of composition here. In other words, derive the ContactListManager from the ContactList and only override functions which need special behavior. This will also elimate the need for any friend declaration.

Don't Repeat Yourself (DRY)

The two pretty_print routines contain a lot of duplicated code, but don't need to. Essentially, there are two distinct pieces: one to find the max width of fields in the collection of records and the other that uses those widths to print the collection of records. I'd suggest that one could use a pair of functions that do those tasks given a particular collection. One could either specify the collection by indices as the code currently does, or perhaps more usefully, by predicate. That is, you could use something like std::for_each

Also, within the print routine itself, the code for printing the header row is almost identical to the code that prints the data records. I suspect that one could easily factor out a function that would serve both needs. I'd probably create a static dummy Contact with all of the header names except "Index" and then define a Contact member function that pretty-prints a record, given a list of field widths.

Where practical, eliminate unused parameters

I understand the desire for a consistent interface, which is good, but when the warning are cranked up on the compiler (as they should normally be), it will tell you that the fieldid parameter is not used in get_optional_field(), and err is not used in get_contact_interactive(). Depending on the particulars of the compiler, one way to address the warning and also to make it clear to readers of the code that you are intentionally not using that parameter, might be to simply omit the name in the definition and implementation. The other obvious alternative is to omit that parameter.

Be careful with signed vs. unsigned

You ask about a compiler warning about possible loss of precision when passing a size_t to setw(). Unfortunately, there's an inherent problem because std::size_t is defined to be an unsigned number, while setw() is defined to take a signed int as a parameter. For any reasonable field width in this application, there is not going to be any loss of precision because any reasonable number can be converted from one form to the other without loss. If you wish to support really, really wide fields, you could add code to assert if the size_t value exceeds std::numeric_limits<int>::max().

Understand move semantics

Since C++11, we have "move semantics" to think about. In the contacts.push_back(c); line you were concerned about, you can explicitly cause a move by using contacts.push_back(std::move(c)); instead or equivalently, contacts.emplace_back(c);

Use standard names

The Contact class has a serialize() function that turns a record into a string. However, there is already a std::to_string() template that one could use instead, or define a cast operator. Doing either of these things helps others better understand your code and also enables your classes to be used more generally. Alternatively, since the only use for this function is printing to a stream, you could define the appropriate ostream &operator<< for the Contact class.

Rethink your search algorithm

The search algorithm is very inefficient. It searches the entire list five times! What would make more sense, I believe, is to create a helper function within the Contact class that returns true if the passed string matches any of the fields. That way, only a single pass through the list would be required.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.