HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

C++11 Command Line Address Book

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
bookcommandaddressline

Problem

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.

-
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 in real code, or are these classes so close
together that you wouldn't bother?

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

-
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?

-
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?

-
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

Solution

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

Code Snippets

friend std::ostream& operator<< (std::ostream& stream, const ContactListManager& csm) {
    for (auto &c : csm.list.contacts) {
        stream << c.serialize() << std::endl;
    }
    return stream;
}
int main(int argc, const char * argv[]) {
int main()
int main(int argc, char *argv[])
int main(int argc, char **argv)
size_t count() {
    return list.count();
}

Context

StackExchange Code Review Q#147940, answer score: 3

Revisions (0)

No revisions yet.