Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[W5.11][F09-2]Salsabil Tasnia Ali Nikita MD #169

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

SalsabilTasnia
Copy link

No description provided.

@kianhong95
Copy link

Good feature to have when user wants to check if the command has been properly executed.

@kangmingtay
Copy link

Great feature to have to enhance the user experience when using the addressbook!

@px1099
Copy link

px1099 commented Sep 13, 2018

Comments:

  • Good improvements, but if the list is too long then it might be hard to see the changes in the list
  • Great use of OOP to implement the improvements
  • User Guide updated to reflect the improvements

Possible Improvements:

  • expected.txt: need to readjust indentation of the added test cases

Copy link

@stephlewyh stephlewyh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Salsabil, good job adding a new enhancement supported by test cases and user documentation. Also, it's great you are reusing code! If the list gets too long, you may consider setting another condition that returns only the recent 50 entries made to the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants