nus-cs2103-AY1718S1 / forum

Discussion Forum
5 stars 0 forks source link

A bug from addressbook-level4 that causes crashes and a simple solution #137

Open newalter opened 6 years ago

newalter commented 6 years ago

The base code from addressbook-level4 handles situations where the xml file storing the addressbook data is corrupted. However, if one deletes a whole line in the xml file, such as <name>Alex Yeoh</name>, the xml file would pass the check and the application will proceed to read in the data. However, now that the name entry of that particular person is gone, when the application tries to convert the data into a Name objective, Name.java throws a NullPointerException which is not handled. Hence, the application would crash. These two PRs provides a solution to the problem: https://github.com/CS2103AUG2017-W13-B3/main/pull/79 https://github.com/CS2103AUG2017-W13-B3/main/pull/98

damithc commented 6 years ago

Thanks for the bug report @newalter @Zhiyuan-Amos can take a look to confirm?

Zhiyuan-Amos commented 6 years ago

Yup, what @newalter said is right. After taking the look at the PR, I think that the fix can be better though :P

Just a hint for @newalter since you showed your PR here: You shouldn't be catching NullPointerException. See https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html. Also, you should fix the problem in the method in XmlAdaptedPerson that's causing the issue, because that's where the bug originates from. ^^

damithc commented 6 years ago

Thanks for checking @Zhiyuan-Amos May be create an issue in se-edu repo and point to this issue? We can fix it at a later date.

newalter commented 6 years ago

@Zhiyuan-Amos Thank you for your hint. I am now tempted to throw an IllegalValueException upon seeing a null argument in Name.java... etc. which will then be handled by the catch block in XmlSerializableAddressBook.getPersonList(). Though the error handling there was not perfected yet.

newalter commented 6 years ago

Updated solution for this issue: https://github.com/CS2103AUG2017-W13-B3/main/pull/79 https://github.com/CS2103AUG2017-W13-B3/main/pull/98