Comments after code review
Opened this issue · 1 comments
brannerchinese commented
Danielle:
A few comments:
- 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). - 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
forchange
. - For
remove
, your usage-docstring prescribes three arguments required, but in practice only two are accepted. - Multiple
add
s 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, applyingchange
changes all of those numbers to the specified new number. And since no phone number can be specified, if I useremove
on a name, it removes all instances of that name, not just a particular one. - 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. - In
change_or_remove
, there is a lot of stuff in thetry
block. I think it's considered best to place as little code as possible in atry
block, so that exceptions are easy to pinpoint. - In
change_or_remove
, I would like to suggest the modular device of having only a singlereturn
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.