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

[T3A4][W09-A3]Yim Chia Hui #117

Closed
wants to merge 2 commits into from

Conversation

sharkey1314
Copy link

Added check for storage file while program is running...

@sharkey1314
Copy link
Author

Ready for review

@hwkchia
Copy link

hwkchia commented Feb 7, 2017

As mentioned during the lecture, the program in the original form continues to work even when the file is deleted by creating a new file; so the use of an exception might be questionable.

Anyway, here is some general feedback on how an exception can be used. There are generally two ways to do this:

  • Terminate if the file is deleted. This is the straightforward approach, but it degrades the user experience. The exception should be thrown from StorageFile::save() method. The best place to catch and terminate is Main::executeCommand() (this method already has a catch block to terminate upon exception.

  • Warn but continue. Need to detect the file deletion at the beginning of the StorageFile::save() method but throw the exception at the end of the method. It should be caught in Main::executeCommand() but should show a warning to the user instead of terminating program.

Also, one should not use exceptions to replace the normal workflow (e.g., using exception in place of if … else)

@hwkchia hwkchia closed this Feb 7, 2017
joelleejh added a commit to joelleejh/addressbook-level2 that referenced this pull request Feb 8, 2017
EdCS2103 pushed a commit to EdCS2103/addressbook-level2 that referenced this pull request Feb 16, 2017
EdCS2103 pushed a commit to EdCS2103/addressbook-level2 that referenced this pull request Feb 16, 2017
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.

2 participants