[#167] Unnecessary catch block in MainApp and Exceptions in Interfaces

[Eclipse-Dominator]

Fixes #167

Alternative Approach: #191

Classes that implements the methods,

• readAddressBook() from AddressBookStorage interface
• readUserPrefs() from UserPrefsStorage interface does not actually throw any IOException as such exceptions are caught within the method and is being treated as DataConversionException which may not be the case.

All IOException are caught by JsonUtil.readJsonFile() which is called by classes implementing these methods.

This method also causes ConfigUtil.readConfig() to not throw IOExceptions when handling actual IOExceptions that is not caused by a conversion error.

This causes the wrong log messages to be logged when an actual IOException occur.

Let's

• decouple actual conversion errors from IOExceptions and restore functionality to the catch blocks in MainApp.java that handles IOExceptions.
• add catch blocks to handle IOException when reading config files

---

We can see the effect of the change below when reading files that does not have sufficient read permissions:

---

Right now, actual IOExceptions (not conversion errors) are still being handled like conversion errors where a blank/default save file is used instead. I would prefer to show some form of warning then exit the application instead. However, this might require additional changes to UI behavior so I left it as such for now.

[canihasreview[bot]]

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

[codecov[bot]]

Codecov Report

Merging #186 (7f075b8) into master (2215c93) will decrease coverage by 0.24%. The diff coverage is 17.64%.

@@             Coverage Diff              @@
##             master     #186      +/-   ##
============================================
- Coverage     72.06%   71.83%   -0.24%     
  Complexity      399      399              
============================================
  Files            70       70              
  Lines          1235     1239       +4     
  Branches        127      127              
============================================
  Hits            890      890              
- Misses          314      318       +4     
  Partials         31       31              

Impacted Files	Coverage Δ
src/main/java/seedu/address/MainApp.java	0.00% <0.00%> (ø)
...in/java/seedu/address/commons/util/ConfigUtil.java	75.00% <ø> (ø)
.../seedu/address/storage/JsonAddressBookStorage.java	100.00% <ø> (ø)
...va/seedu/address/storage/JsonUserPrefsStorage.java	87.50% <ø> (ø)
...main/java/seedu/address/commons/util/JsonUtil.java	86.48% <75.00%> (-7.64%)	:arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[canihasreview[bot]]

v1

@Eclipse-Dominator submitted v1 for review.

• [v1 1/1] Restore functionality to unreachable catch blocks

(:books: Archive)

Checkout this PR version locally

```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/186/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.

[canihasreview[bot]]

v2

@Eclipse-Dominator submitted v2 for review.

• [v2 1/1] Restore functionality to unreachable catch blocks

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v1 and v2) (:chart_with_upwards_trend: Range-Diff between v1 and v2)

Checkout this PR version locally

```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/186/2/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.

[Eclipse-Dominator]

I have included some change to reduce code duplication in handling of config and pref files.

[damithc]

@se-edu/tech-team-level1 for your reviews please ...

[damithc]

Upon reviewing this Pull Request (PR), it appears that it may not adequately address the problem outlined in issue https://github.com/se-edu/addressbook-level3/issues/167. Specifically, the changes seem to make the IOException catch block mandatory in MainApp, which may not be the intended solution.

I agree what what @SPWwj said above in that there is disparity between the issue and the PR. The original intention was to remove the unnecessary catch block. I prefer that approach as it simplifies the code without any loss of value (because we do not currently have different handlings for incorrect format in JSON files vs other I/O problems with the JSON files).

In addition, The name DataConversionException may be misleading because we use it to represent both kind of problems mentioned above. Perhaps something like ConfigLoadingException is more representative?

[SPWwj]

DataConversionException

The renaming of the exception to ConfigLoadingException aligns well with its purpose.

In JsonUtil.java, handling the IOException by logging it provides valuable information about the location of the error, aiding in debugging.

In MainApp, handling the ConfigLoadingException manages any issues that arise during the data fetching process, enhancing the robustness of error handling and preventing application crashes due to unhandled exceptions.

The use of interfaces for the readUserPrefs method allows for various implementations of the logic. It's not restricted to reading data from a local JSON file; it could also fetch data over the internet. As a result, different types of exceptions, not just IOException, could be encountered. By using ConfigLoadingException as a general exception, these scenarios can be handled in a unified way.

[Eclipse-Dominator]

Upon reviewing this Pull Request (PR), it appears that it may not adequately address the problem outlined in issue #167. Specifically, the changes seem to make the IOException catch block mandatory in MainApp, which may not be the intended solution.

I agree what what @SPWwj said above in that there is disparity between the issue and the PR. The original intention was to remove the unnecessary catch block. I prefer that approach as it simplifies the code without any loss of value (because we do not currently have different handlings for incorrect format in JSON files vs other I/O problems with the JSON files).

In addition, The name DataConversionException may be misleading because we use it to represent both kind of problems mentioned above. Perhaps something like ConfigLoadingException is more representative?

Hmm, it's actually possible to remove the unnecessary catch block as indicated in the original PR as technically IO related errors/logs can be handled within the jsonutil method and we only need to mention that something went wrong while trying to read XXX file in the MainApp.

My original concern is that students may want to implement additional features/error handling when encountering these errors. Thus, I went ahead with restoring the functionality of the IOException catch block.

As this is also another possibility, I will create a separate PR removing the catch blocks as intended in the original issue to explore other approaches to this. You can find the alternative approach in #191

[damithc]

Closing this in favor of #191