dphm/phonebook

Comments after code review

Opened this issue · 1 comments

Danielle:

A few comments:

  1. There are some formatting curiosities: I think PEP8 specifies docstrings after the def line rather than before, and no blank lines within functions (you can use an otherwise-empty # line to create visual break).
  2. Running instructions in main docstring would be usefully placed in README. And your use of square brackets to represent an optional value should probably be explained explicitly, since the brackets are not part of the syntax. Also: Usage-docstring has typo changed for change.
  3. For remove, your usage-docstring prescribes three arguments required, but in practice only two are accepted.
  4. Multiple adds of the same name don't generate a duplicate-entry error, which I think the spec prescribes (see line 11 of the spec example usage). A consequence of that: If I have multiple numbers for users with the same name, applying change changes all of those numbers to the specified new number. And since no phone number can be specified, if I use remove on a name, it removes all instances of that name, not just a particular one.
  5. It would probably be useful to assume a default phonebook name — that would save the user some work typing the name in each time. However, I find that using *.pb works as long as there's only one in the directory.
  6. In change_or_remove, there is a lot of stuff in the try block. I think it's considered best to place as little code as possible in a try block, so that exceptions are easy to pinpoint.
  7. In change_or_remove, I would like to suggest the modular device of having only a single return statement, so that it's easy for the reader to see where the function returns. You could supply different values of, say, return_message in order to have different return values.

[end]

dphm commented

Thank you for the feedback! 😄 I definitely want to address all of these points in turn as my main goal this morning was to get something working.