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

Fix error message to fit desired usage for indexing of integers larger than max int #164

Merged
merged 9 commits into from
Nov 7, 2021

Conversation

ykwei7
Copy link

@ykwei7 ykwei7 commented Nov 3, 2021

Resolves #116, #123, #130, #135, #142

Resolves get, remark, deletelesson, edit and clearremark

Adjusted ParserUtil to break down indexing into 2 cases:

  1. When input is valid integer and is larger than max_int
  2. When input is non-valid

When input < max_int
image
When input > max_int
image
When input wiithin list range
image
When input is empty
image

@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #164 (349b95c) into master (2691b2c) will increase coverage by 0.65%.
The diff coverage is 80.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #164      +/-   ##
============================================
+ Coverage     79.44%   80.10%   +0.65%     
- Complexity      697      714      +17     
============================================
  Files            95       96       +1     
  Lines          2097     2151      +54     
  Branches        258      264       +6     
============================================
+ Hits           1666     1723      +57     
+ Misses          361      358       -3     
  Partials         70       70              
Impacted Files Coverage Δ
...main/java/seedu/address/commons/core/Messages.java 0.00% <ø> (ø)
...rc/main/java/seedu/address/logic/LogicManager.java 75.00% <ø> (ø)
.../java/seedu/address/logic/parser/TrackOParser.java 100.00% <ø> (ø)
src/main/java/seedu/address/ui/MainWindow.java 0.00% <0.00%> (ø)
...c/parser/exceptions/IndexOutOfBoundsException.java 50.00% <50.00%> (ø)
...in/java/seedu/address/logic/parser/ParserUtil.java 91.86% <83.33%> (-0.80%) ⬇️
...u/address/logic/parser/AddLessonCommandParser.java 100.00% <100.00%> (ø)
...address/logic/parser/ClearRemarkCommandParser.java 100.00% <100.00%> (ø)
...eedu/address/logic/parser/DeleteCommandParser.java 100.00% <100.00%> (ø)
...ddress/logic/parser/DeleteLessonCommandParser.java 100.00% <100.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2691b2c...349b95c. Read the comment docs.

* @param s
* @return
*/
private static boolean isAllDigits(String s) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a regex check suffices here. return s.matches("^\\d+$")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! Have replaced the existing function with this regex.

String trimmedIndex = oneBasedIndex.trim();
if (!StringUtil.isNonZeroUnsignedInteger(trimmedIndex)) {

if (!isAllDigits(trimmedIndex) || trimmedIndex.equals("0") || trimmedIndex.equals("")) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does isAllDigits cover the case whereby trimmedIndex.equals("")?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have removed the unnecessary condition, thanks for pointing out!

@@ -36,16 +37,59 @@

public static final String MESSAGE_INVALID_INDEX = "Index is not a non-zero unsigned integer.";

public static final String MESSAGE_INDEX_OUT_OF_BOUNDS = "The index provided is invalid";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor nitpick, remember to add in a full stop here 😅

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks for the keen observation!

@@ -191,6 +194,10 @@ private CommandResult executeCommand(String commandText) throws CommandException
logger.info("Invalid command: " + commandText);
resultDisplay.setFeedbackToUser(e.getMessage());
throw e;
} catch (IndexOutOfBoundsException ie) {
logger.info("Invalid index: " + commandText);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of logging. 👍

@atyhamos atyhamos merged commit 58a8172 into AY2122S1-CS2103T-F12-3:master Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment