-
Notifications
You must be signed in to change notification settings - Fork 5
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
Bug Fixes #166
Bug Fixes #166
Conversation
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.
Perhaps some minor changes needed.
@@ -114,6 +114,8 @@ public static void main(String[] args) { | |||
c.execute(dietBook.manager, dietBook.ui); | |||
} catch (DietException e) { | |||
dietBook.ui.printErrorMessage(e.getMessage()); | |||
} catch (Exception e) { | |||
throw new DietException("Wrong command format!"); |
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.
Perhaps consider changing to a more generic message.
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 feel like this is super generic already because all the input are all "commands" so any typos should be safe to consider as a wrong format of command. "Something went wrong" is too vague of a message i feel? wdyt
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 see. I was thinking that since all the exceptions are being caught here it could also possibly be catching an error from elsewhere in the system other than the command input. However, if you think that only the command input is likely to result in an exception that is caught here I think you can leave it as such.
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.
Hmm runtime error and such rite,,,, orite i will just change it to Something went wrong cos i think we will be catching most of the exceptions for commands.
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.
Perhaps you can consider catching other possible generic exceptions that may be thrown like NumberFormatException, StringIndexOutOfBound, IOException before catching Exception so that more specific error messages can be output.
} else if (age > AGE_CAP) { | ||
throw new DietException("Input value cannot be more than 125!"); | ||
throw new DietException("Age value cannot be more than 125!"); |
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.
Perhaps consider changing 125 to AGE_CAP or 150. You might like to consider this suggestion for the other methods as well.
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.
Perhaps consider closing the relevant issues reported in the PE dry run when merging this.
Current commit LGTM.
- Specific exception messages
-Catching all exception
-Fixed for specific exceptions
-Fixed calculate rubbish acceptance
-Fixed name accepting empty/empty checker