patterncppMinor
C++11 Command Line Address Book
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
(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
I should be able to expose a contacts iterator without letting
you expose that? Would you bother to hide the fact that
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.
-
-
Specifically I am getting compiler warnings
losing integer precision when I pass
about that? Is there a way to get rid of them by maxing them. What I
really want to is 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.
find
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 wouldyou expose that? Would you bother to hide the fact that
ContactList is a glorifiedstd::vector in real code, or are these classes so closetogether 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 tostd::vector as well? -
Specifically I am getting compiler warnings
losing integer precision when I pass
size_t to setw. What would you doabout 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 tocast 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'tfind
Solution
Here are some things that may help you improve your code.
Fix the bugs
The code currently contains this function:
However, this won't compile on my machine because
Another bug is that because only sorting by last name sets the
Yet another bug is that
Use the standard form of
The current
However, the use of
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
The difference betweeen
Reconsider your class design
The
This strongly suggests that it would probably be better to use inheritance instead of composition here. In other words, derive the
Don't Repeat Yourself (DRY)
The two
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
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
Be careful with signed vs. unsigned
You ask about a compiler warning about possible loss of precision when passing a
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 itThe 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_eachAlso, 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.