-
Notifications
You must be signed in to change notification settings - Fork 427
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
[#167] Removal of unnecessary catch blocks #191
[#167] Removal of unnecessary catch blocks #191
Conversation
Click here to submit a new iteration when this PR is ready for review. See this repository's contribution guide for more information. |
v1@Eclipse-Dominator submitted v1 for review.
Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/191/1/head:BRANCHNAME where |
Codecov Report
@@ Coverage Diff @@
## master #191 +/- ##
============================================
+ Coverage 72.06% 72.41% +0.35%
Complexity 399 399
============================================
Files 70 70
Lines 1235 1229 -6
Branches 127 127
============================================
Hits 890 890
+ Misses 314 308 -6
Partials 31 31
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick feedback: The renaming of the exception class deserves its own commit?
81c3cf7
to
04749db
Compare
v2@Eclipse-Dominator submitted v2 for review.
(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/191/2/head:BRANCHNAME where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions added.
@@ -1,7 +1,7 @@ | |||
package seedu.address.commons.exceptions; | |||
|
|||
/** | |||
* Represents an error during conversion of data from one format to another | |||
* Represents an error during loading of files from the file system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this belongs in the other commit, together with the name change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking, the correct order is to tweak the exception first, and then remove the catch blocks. The latter is more justifiable after the exception has been tweaked.
* @param filePath cannot be null. | ||
* @param classOfObjectToDeserialize Json file has to correspond to the structure in the class given here. | ||
* @throws DataConversionException if the file format is not as expected. | ||
* @throws DataConversionException if the loading of file failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if loading of the file
will be understood as both 'reading the file content and parsing the content'. Perhaps use loading of JSON data from the file
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this version of PR, the handling of trying to read file content and parsing the content is both handled within JsonUtil
so in a sense the new renamed exception represents both 'reading the file content and parsing the content'.
* @param prefsFilePath location of the data. Cannot be null. | ||
* @throws DataConversionException if the file format is not as expected. | ||
* @throws DataConversionException if the loading of the preference file fails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is phrasing is slightly different from the one used in readAddressBook
. Don't vary things unless there is a good reason.
@@ -40,7 +40,7 @@ public Optional<ReadOnlyAddressBook> readAddressBook() throws DataConversionExce | |||
* Similar to {@link #readAddressBook()}. | |||
* | |||
* @param filePath location of the data. Cannot be null. | |||
* @throws DataConversionException if the file is not in the correct format. | |||
* @throws DataConversionException if the loading of the data fails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fails -> failed?
@@ -68,7 +68,8 @@ private Config getTypicalConfig() { | |||
return config; | |||
} | |||
|
|||
private Optional<Config> read(String configFileInTestDataFolder) throws DataConversionException { | |||
private Optional<Config> read(String configFileInTestDataFolder) | |||
throws DataConversionException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not needed?
@@ -47,17 +47,20 @@ public void read_missingFile_emptyResult() throws Exception { | |||
|
|||
@Test | |||
public void read_notJsonFormat_exceptionThrown() { | |||
assertThrows(DataConversionException.class, () -> readAddressBook("notJsonFormatAddressBook.json")); | |||
assertThrows(DataConversionException.class, | |||
() -> readAddressBook("notJsonFormatAddressBook.json")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert unrelated changes.
eaf1589
to
7839b9a
Compare
v3@Eclipse-Dominator submitted v3 for review.
(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/191/3/head:BRANCHNAME where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first commit should be just the renaming. It is a mechanical change and requires minimal review. The only other thing that can be go into that commit is the tweaking of the javadoc comment of that class so that the comment matches the new class name.
Once that is out of the way, the next commit can focus on behavioral tweaks.
src/main/java/seedu/address/commons/exceptions/ConfigLoadingException.java
Outdated
Show resolved
Hide resolved
7839b9a
to
8609d5a
Compare
v4@Eclipse-Dominator submitted v4 for review.
(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/191/4/head:BRANCHNAME where |
8609d5a
to
15fd221
Compare
v5@Eclipse-Dominator submitted v5 for review.
(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/191/5/head:BRANCHNAME where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly OK. I suggest the following:
- Commit 1: Remove everything else except those strictly related to renaming the exception. It should look almost like a mechanical find-and-replace refactoring.
- Commit 2: commit message: No need to mention 'In the previous commit'. The previous commit speaks for itself. Here, mention that IOException is unnecessary and is removed, and logs/docs are adjusted accordingly.
15fd221
to
f2f77c7
Compare
DataConversionException is thrown to indicate any problem encountered while reading a JSON file, not just problems specific to data conversion. Let's rename it as DataLoadingException to make the name more representative of its purpose.
f2f77c7
to
5ccd0d6
Compare
v6@Eclipse-Dominator submitted v6 for review.
(📚 Archive) (📈 Interdiff between v5 and v6) (📈 Range-Diff between v5 and v6) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/191/6/head:BRANCHNAME where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 1: Clean and easy to review. Nice!
Commit 2: Added a few cosmetic suggestions.
@Eclipse-Dominator Can you update the PR description to match the current state of the PR? After that, I'll ask others for reviews. |
5ccd0d6
to
c5e45e8
Compare
JsonUtil#readJsonFile() method throws a DataLoadingException when it detects an IOException. However, some client code of that method are written as if it throws an IOException. Let's, 1. remove 'throws IOExceptions' clause from the relevant client methods in Storage, AddressBookStorage, and UserPrefsStorage. 2. remove corresponding catch blocks. 3. tweak relevant javadocs and log message to match.
c5e45e8
to
e00047a
Compare
v7@Eclipse-Dominator submitted v7 for review.
(📚 Archive) (📈 Interdiff between v6 and v7) (📈 Range-Diff between v6 and v7) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/191/7/head:BRANCHNAME where |
@se-edu/tech-team-level1 for your review ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Now, the MainApp.java has become more streamlined, and the exceptions are more specific. Excellent work!
Thanks @Eclipse-Dominator for the fix, and @SPWwj for the review. |
Fixes #167
Alternative approach: #186 (Where functionality of the catch blocks is restored)
Following issue #167, all IOExceptions are already being handled within
JsonUtil#readJsonFile()
, therefore additional catch blocks as well as throwing of IOExceptions are no longer needed.This PR moves handling of conversion errors as well as regular IOExceptions to within
JsonUtil#readJsonFile()
. As a result of this change, it is no longer appropriate to call the exception thrownDataConversionException
.Thus, the above-mentioned exception will be renamed to
DataLoadingException
. Affected interfaces and code caused by this change will then be updated.