yadobler / pe

0 stars 0 forks source link

Inconsistent/Incorrect error message when invalid index is given #6

Open yadobler opened 2 weeks ago

yadobler commented 2 weeks ago

Summary

When the Index for a command (e.g. edit) is 0, the error given is related to "invalid command format", but when the Index given is more than the number of items listed, the error given is instead "The person index is invalid".

Steps to reproduce

  1. Type edit 0 n/test
  2. Observe error
  3. Type edit 100 n/test given the number of persons listed is less than 100
  4. Observe error

Expected

Both result in "The person index is invalid".

Observed

edit 0 n/test

image.png

edit 100 n/test

image.png

Tester Information

System Information:

Java version:

nus-se-script commented 1 week ago

Team's Response

Thanks for mentioning this issue, I think it's mentioned in the User Guide under note of the command edit. Because index is positive integer, so 0 is considered as invalid command, while out of bound positive integers are considered invalid index.

image.png

Items for the Tester to Verify

:question: Issue response

Team chose [response.NotInScope]

Reason for disagreement: ### As a user

  1. Invalid command format = I did not format my edit command correctly, but I did format edit <index> <editable parameter> correctly:
    1. I gave a preamble as documented (0).
    2. I gave a valid prefix (n/).
    3. I did not give any invalid prefix.
  2. "The person index provided is invalid" = The edit command format is correct, but the index preamble given is invalid because "Index is a positive integer".
  3. Being a user with fat fingers, if I'm editing some contact and the command is very very very long (e.g. I'm editing address and phone and everything), and I get "Invalid command format", I don't want to play treasure hunting and find what when wrong. I also don't want a red herring that makes me check if I used a wrong prefix or have duplicate prefixes or something. I would appreciate if I am told what the exact error was.
  4. if "(edit) 0 is considered as invalid command", why is edit 1 n/!@#!@# not an invalid command, but instead a valid command with a specific "incorrect name parameter" error message:

    image.png

    This would also be an invalid command since "name" is alphanumeric+space, just like how "index" is a positive integer. This actually confused me more since I am incorrectly led to assume that if the prefixed parameters have customised validity errors, then "index" would also have one (or that the "The person index provided is invalid" error would apply to this as well - since it is not specifically that "There exists no user at index X" but a more general "index invalid".

  5. Typing 0 as index is not a harmful input since:
    1. I can accidentally type 0 instead of 9 as a typo; or
    2. since there is a large overlap between programmers and gamers, the first item might be mistakenly thought of as 0. This ties back to (3) where I would not realise the 0 is actually the issue. Since the error states the "command format" is incorrect, the user will be focused on the format itself, and skip over the note stating the index is positive integer.
  6. This note portion also does not explicitly say 0 is invalid, just that the index is a value like 1, 2, 3,... and relies on the user to understand the meaning of "positive integer".

As a developer:

  1. The points (3) and (4) is an example that the first user story is not properly satisfied since the new user (me) is not guided properly image.png
  2. There's no use case MSS for editing contacts.
  3. Looking at the "Logic Component" of the DG, I presume that both "Invalid Command" and "Invalid Index" are stemming from failure to generate a valid Command interface object in the CommandParser interface class (specifically editCommand and editCommandParser). The preamble from the ArgumentTokenizer seems to be unable to handle the index 0 and errors out but caught as an exception while collecting the arguments - as opposed to after collecting and passing the tokens into the ParserUtil where individual checks happen and "Invalid Index" is thrown. (that is my understanding from the DG). This means that there is a code smell somewhere, and edge cases like point (5) are not caught or anticipated.