Removal of repeatable parameters #190

Eclipse-Dominator · 2023-06-06T23:39:43Z

This PR an alternative that Fixes #162 (other alternatives: #176, #181)

In this PR, we treat repeated parameters as an error.

If a single-valued parameter is repeated multiple times in a 
command, only the last occurrence is taken.  This allows the user
to correct a mistake in a parameter value by simply typing it again,
which may be faster than moving the cursor back to the original
parameter values and editing it. This feature serves as a good
example of 'going the extra mile to optimize the application for
target users' (i.e., users who can type fast).
However, this feature can cause certain user errors to go unnoticed
e.g., unintended repetition caused by a typo in parameter name

Alternatives considered:
(a) Retain current behavior but give a friendly warning to the user
    that a parameter was 'overridden'. 
(b) Remove this feature i.e., treat repeated single-valued
    parameters    as an error, and reject the command with an
    error message.

Alternative (a) requires significantly more complicated code
(see #181), steepening the learning curve for for new programmers.
Given that this application is primarily meant for training
novice programmers, the impact on the learning curve is not worth
the benefit of retaining this feature.

Therefore, let's do option (b).

canihasreview · 2023-06-06T23:39:46Z

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

See this repository's contribution guide for more information.

codecov · 2023-06-06T23:41:49Z

Codecov Report

Merging #190 (4a4144d) into master (bd48d59) will increase coverage by 0.16%.
The diff coverage is 91.66%.

@@             Coverage Diff              @@
##             master     #190      +/-   ##
============================================
+ Coverage     74.00%   74.16%   +0.16%     
- Complexity      420      428       +8     
============================================
  Files            71       71              
  Lines          1281     1293      +12     
  Branches        126      127       +1     
============================================
+ Hits            948      959      +11     
+ Misses          301      300       -1     
- Partials         32       34       +2

Impacted Files	Coverage Δ
src/main/java/seedu/address/logic/Messages.java	`87.50% <75.00%> (-4.17%)`	⬇️
...a/seedu/address/logic/parser/AddCommandParser.java	`100.00% <100.00%> (ø)`
...a/seedu/address/logic/parser/ArgumentMultimap.java	`100.00% <100.00%> (ø)`
.../seedu/address/logic/parser/EditCommandParser.java	`92.59% <100.00%> (+0.28%)`	⬆️

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

canihasreview · 2023-06-07T07:56:52Z

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Removal of repeatable parameters

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

canihasreview · 2023-06-07T07:56:52Z

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/1] Removal of repeatable parameters

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc · 2023-06-07T17:29:06Z

@Eclipse-Dominator Now that we have three alternatives, what your thoughts about selecting one?
Also, in each of the three PR descriptions, you can mention the other two so that anyone visiting the PR can see how they fit into the big picture.

damithc · 2023-06-08T11:17:49Z

Repeated Parameters show warning: #176
Repeated Parameters show warning of repeated prefixes: #181
Repeated Parametes treated as errors: #190

Updated the PR description instead.

damithc

Some initial comments ...

src/main/java/seedu/address/logic/parser/EditCommandParser.java

src/main/java/seedu/address/logic/parser/AddCommandParser.java

src/test/java/seedu/address/logic/parser/AddCommandParserTest.java

damithc · 2023-06-12T17:22:01Z

@Eclipse-Dominator As discussed, let's proceed with this approach.

damithc

Added a few more comments.

src/main/java/seedu/address/commons/core/Messages.java

src/main/java/seedu/address/logic/parser/ArgumentMultimap.java

src/test/java/seedu/address/logic/parser/AddCommandParserTest.java

canihasreview · 2023-06-12T22:09:26Z

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/1] Removal of repeatable parameters as feature

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

jundatan

Suggestion to change the message for duplicate person.

jundatan · 2023-06-13T03:57:56Z

src/main/java/seedu/address/commons/core/Messages.java

@@ -9,5 +14,21 @@ public class Messages {
    public static final String MESSAGE_INVALID_COMMAND_FORMAT = "Invalid command format! \n%1$s";
    public static final String MESSAGE_INVALID_PERSON_DISPLAYED_INDEX = "The person index provided is invalid";
    public static final String MESSAGE_PERSONS_LISTED_OVERVIEW = "%1$d persons listed!";
+    public static final String MESSAGE_DUPLICATE_FIELDS = "Detected multiple values for the following field%s: "
+            + "%s.\nPlease only a single value for those fields\n";


I feel that this Message should be worded properly. Perhaps it will be better to have a message like "Multiple values detected for the following field%s: " + "%s.\nOnly one value is allowed for these fields\n".

canihasreview · 2023-06-13T09:35:53Z

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/1] Removal of repeatable parameters as feature

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc

Some suggestions added

damithc · 2023-06-13T15:28:37Z

src/main/java/seedu/address/commons/core/Messages.java

+
+        return String.format(MESSAGE_DUPLICATE_FIELDS, duplicateFields.size() > 1 ? "s" : "",
+                String.join(" ", duplicateFields), duplicateFields.size() > 1 ? "these" : "this");
+    }


I think we are complicating this code too much for too little payback. No need to bother with singular/plural differences. Something like the following?

Multiple values specified for the following single-valued filed(s): ...

Personally, I think over complicating this code is okay here since everything related to the messages have been abstracted to within Messages. So, there won't be any additional code duplication and its self-contained to within Messages.

That being said, I think changing it the message to

Multiple values specified for the following single-valued field(s): ... Please only supply a single value for the field(s) mentioned above.

can make it appear more user-friendly?

Personally, I think over complicating this code is okay here since everything related to the messages have been abstracted to within Messages. So, there won't be any additional code duplication and its self-contained to within Messages.

Always be as economical as possible with code. Every line of code is an additional burden every project member has to carry for the rest of the lifetime of the project.

That being said, I think changing it the message to

Multiple values specified for the following single-valued field(s): ...
Please only supply a single value for the field(s) mentioned above.
can make it appear more user-friendly?

Not totally opposed, but I have some concerns:

Feels a bit redundant. Check how the UI present this error to the user. Perhaps the first sentence is enough for the user to understand the problem.

Check other error messages. If they don't have Please do such and such ... part, we don't want to make this specific error message different from others.

src/main/java/seedu/address/logic/parser/ArgumentMultimap.java

damithc · 2023-06-13T15:40:23Z

src/main/java/seedu/address/commons/core/Messages.java

+    /**
+     * Formats the duplicate prefixes into an error message.
+     */
+    public static String getDuplicatePrefixesToMessage(Set<Prefix> duplicatePrefixes) {


alternative to consider:

Suggested change

public static String getDuplicatePrefixesToMessage(Set<Prefix> duplicatePrefixes) {

public static String getDuplicatePrefixesToMessage(Prefix... duplicatePrefixes) {

I kept it intentionally this to avoid ordering of the fields in the message to avoid complications when doing unit test etc.
It is possible to sort the field within this method but I feel that supplying it with a Set will make it cleaner.

Instead of creating Set objects everywhere this method is called, you can create the Set inside the method at the start?

Yes, but the code technically is really only called once in production code and the rest of it is called in unit test.

Which is called below.

/** * Throws a {@code ParseException} if any of the prefixes given in {@code prefixes} appeared more than * once among the arguments. */ public void verifyNoDuplicatePrefixesFor(Prefix... prefixes) throws ParseException { Set<Prefix> duplicatedPrefixes = new HashSet<>(); Set<Prefix> prefixSet = Set.of(prefixes); argMultimap.forEach((key, list) -> { if (list.size() > 1 && prefixSet.contains(key)) { duplicatedPrefixes.add(key); } }); if (!duplicatedPrefixes.isEmpty()) { throw new ParseException(Messages.getDuplicatePrefixesToMessage(duplicatedPrefixes)); } }

Converting getDuplicatePrefixesToMessage() to take in Prefix... or Prefix[] will involve calling duplicatedPrefixes.toArray(Prefix[]::new)) which will cause it to be converted once again into a Set object when inside the method.

This is true unless we are using a fixed-size array for duplicatedPrefixes or alternative we can convert the whole process as a java stream and convert it to an array.

I will experiment processing the above with java stream and see its readability in the next version.

I think the code is cleaner with java stream, I will update the relevant code.

BTW, you are doing something very similar in here:

public void verifyNoDuplicatePrefixesFor(Prefix... prefixes) throws ParseException { Set<Prefix> duplicatedPrefixes = new HashSet<>(); Set<Prefix> prefixSet = Set.of(prefixes);

canihasreview · 2023-06-14T03:15:37Z

v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/1] Removal of repeatable parameters as feature

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/4/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

canihasreview · 2023-06-14T03:25:24Z

v5

@Eclipse-Dominator submitted v5 for review.

[v5 1/2] Removal of repeatable parameters as feature
[v5 2/2] As with previous commit, the user guide should be updated accordingly to indicate that repeating parameters
will be considered as an invalid command in the application

(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/5/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

canihasreview · 2023-06-14T03:26:53Z

v6

@Eclipse-Dominator submitted v6 for review.

[v6 1/2] Removal of repeatable parameters as feature
[v6 2/2] Update User Guide for repeated command

(📚 Archive) (📈 Interdiff between v5 and v6) (📈 Range-Diff between v5 and v6)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/6/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc

Some further suggestions

docs/UserGuide.md

src/main/java/seedu/address/commons/core/Messages.java

src/test/java/seedu/address/logic/parser/EditCommandParserTest.java

canihasreview · 2023-07-03T12:34:12Z

v10

@Eclipse-Dominator submitted v10 for review.

[v10 1/1] Removal of repeatable parameters as a feature

(📚 Archive) (📈 Interdiff between v9 and v10) (📈 Range-Diff between v9 and v10)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/10/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Eclipse-Dominator · 2023-07-03T12:34:54Z

Resolve rebase conflicts.

damithc · 2023-07-03T15:10:07Z

@Eclipse-Dominator Looks like the rebase wasn't entirely correct. There are some unrelated code in this PR.

canihasreview · 2023-07-10T13:11:02Z

v11

@Eclipse-Dominator submitted v11 for review.

[v11 1/1] Removal of repeatable parameters as a feature

(📚 Archive) (📈 Interdiff between v10 and v11) (📈 Range-Diff between v10 and v11)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/11/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

src/test/java/seedu/address/logic/parser/EditCommandParserTest.java

docs/UserGuide.md

canihasreview · 2023-07-13T00:37:50Z

v12

@Eclipse-Dominator submitted v12 for review.

[v12 1/1] Removal of repeatable parameters as a feature

(📚 Archive) (📈 Interdiff between v11 and v12) (📈 Range-Diff between v11 and v12)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/12/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc

Some comments added (did not review the test code though).

docs/UserGuide.md

src/main/java/seedu/address/commons/core/Messages.java

src/main/java/seedu/address/logic/parser/ArgumentMultimap.java

src/main/java/seedu/address/commons/core/Messages.java

canihasreview · 2023-07-13T13:42:46Z

v13

@Eclipse-Dominator submitted v13 for review.

[v13 1/1] Remove support for repeated parameters in commands

(📚 Archive) (📈 Interdiff between v12 and v13) (📈 Range-Diff between v12 and v13)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/13/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc

Just a minor question. I think we are close to home on this one. Let me ask others to have a look as well.

src/test/java/seedu/address/logic/parser/EditCommandParserTest.java

damithc · 2023-07-13T13:59:58Z

@se-edu/tech-team-level1 for your review ...

dlimyy

Overall, LGTM! Just some minor suggestions below

src/main/java/seedu/address/logic/parser/ArgumentMultimap.java

dlimyy · 2023-07-14T07:13:52Z

src/main/java/seedu/address/logic/parser/ArgumentMultimap.java

+     * Throws a {@code ParseException} if any of the prefixes given in {@code prefixes} appeared more than
+     * once among the arguments.
+     */
+    public void verifyNoDuplicatePrefixesFor(Prefix... prefixes) throws ParseException {


Could consider shortening the name of the method to verifyNoDuplicatePrefixes as the 'For' does not seem to be very necessary

It reads as a natural statement e.g., verifyNoDuplicatePrefixesFor(PREFIX_NAME, PREFIX_ADDRESS) i.e., verify no duplicate prefixes for A, B, ..
But OK to remove.
Can also consider verifyNoDuplicatesFor as Prefix is implied by the parameter type.

src/test/java/seedu/address/logic/parser/EditCommandParserTest.java

docs/UserGuide.md

src/main/java/seedu/address/logic/Messages.java

canihasreview · 2023-07-14T12:16:22Z

v14

@Eclipse-Dominator submitted v14 for review.

[v14 1/1] Remove support for repeated parameters in commands

(📚 Archive) (📈 Interdiff between v13 and v14) (📈 Range-Diff between v13 and v14)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/14/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc

Two commented you missed.

src/main/java/seedu/address/logic/parser/ArgumentMultimap.java

src/main/java/seedu/address/logic/Messages.java

If a single-valued parameter is repeated multiple times in a command, only the last occurrence is taken. This allows the user to correct a mistake in a parameter value by simply typing it again, which may be faster than moving the cursor back to the original parameter values and editing it. This feature serves as a good example of 'going the extra mile to optimize the application for target users' (i.e., users who can type fast). However, this feature can cause certain user errors to go unnoticed e.g., unintended repetition caused by a typo in parameter name Alternatives considered: (a) Retain current behavior but give a friendly warning to the user that a parameter was 'overridden'. (b) Remove this feature i.e., treat repeated single-valued parameters as an error, and reject the command with an error message. Alternative (a) requires significantly more complicated code (see se-edu#181), steepening the learning curve for for new programmers. Given that this application is primarily meant for training novice programmers, the impact on the learning curve is not worth the benefit of retaining this feature. Therefore, let's do option (b).

canihasreview · 2023-07-14T14:23:10Z

v15

@Eclipse-Dominator submitted v15 for review.

[v15 1/1] Remove support for repeated parameters in commands

(📚 Archive) (📈 Interdiff between v14 and v15) (📈 Range-Diff between v14 and v15)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/15/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

SPWwj

Code works as expected. LGTM.

Eclipse-Dominator mentioned this pull request Jun 7, 2023

[#162] Show warning when ignoring repeated (Alternative approach) #181

Closed

canihasreview bot added s.ToReview labels Jun 7, 2023

Eclipse-Dominator mentioned this pull request Jun 7, 2023

[#162] Show warning when ignoring repeated parameters #176

Closed

se-edu deleted a comment from Eclipse-Dominator Jun 8, 2023

damithc reviewed Jun 8, 2023

View reviewed changes

damithc requested changes Jun 12, 2023

View reviewed changes

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch from ba48384 to 7d3bb2b Compare June 12, 2023 22:08

canihasreview bot requested a review from damithc June 12, 2023 22:09

jundatan reviewed Jun 13, 2023

View reviewed changes

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch from 7d3bb2b to 6210dce Compare June 13, 2023 09:31

damithc reviewed Jun 13, 2023

View reviewed changes

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch from 6210dce to a4b0fa9 Compare June 14, 2023 03:15

canihasreview bot requested a review from damithc June 14, 2023 03:15

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch from dd943c5 to 9d63a65 Compare June 14, 2023 03:26

damithc requested changes Jun 14, 2023

View reviewed changes

docs/UserGuide.md Outdated Show resolved Hide resolved

src/main/java/seedu/address/commons/core/Messages.java Outdated Show resolved Hide resolved

src/test/java/seedu/address/logic/parser/EditCommandParserTest.java Show resolved Hide resolved

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch 2 times, most recently from 84bc1cd to 1e74f73 Compare June 14, 2023 05:57

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch from d7b3553 to 7841d23 Compare July 10, 2023 13:09

damithc reviewed Jul 10, 2023

View reviewed changes

src/test/java/seedu/address/logic/parser/EditCommandParserTest.java Outdated Show resolved Hide resolved

src/test/java/seedu/address/logic/parser/EditCommandParserTest.java Outdated Show resolved Hide resolved

docs/UserGuide.md Show resolved Hide resolved

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch from 7841d23 to e407b81 Compare July 13, 2023 00:11

canihasreview bot requested a review from damithc July 13, 2023 00:37

damithc requested changes Jul 13, 2023

View reviewed changes

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch 3 times, most recently from 938be75 to ce1f59b Compare July 13, 2023 13:37

canihasreview bot requested a review from damithc July 13, 2023 13:42

damithc reviewed Jul 13, 2023

View reviewed changes

src/test/java/seedu/address/logic/parser/EditCommandParserTest.java Outdated Show resolved Hide resolved

dlimyy reviewed Jul 14, 2023

View reviewed changes

SPWwj suggested changes Jul 14, 2023

View reviewed changes

docs/UserGuide.md Show resolved Hide resolved

src/main/java/seedu/address/logic/Messages.java Show resolved Hide resolved

src/main/java/seedu/address/logic/Messages.java Outdated Show resolved Hide resolved

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch from ce1f59b to 3d2c2df Compare July 14, 2023 12:09

canihasreview bot requested a review from damithc July 14, 2023 12:16

damithc requested changes Jul 14, 2023

View reviewed changes

src/main/java/seedu/address/logic/parser/ArgumentMultimap.java Outdated Show resolved Hide resolved

src/main/java/seedu/address/logic/Messages.java Outdated Show resolved Hide resolved

Eclipse-Dominator force-pushed the change-repeated-parameter-to-error branch from 3d2c2df to 4a4144d Compare July 14, 2023 14:22

canihasreview bot requested a review from damithc July 14, 2023 14:23

SPWwj approved these changes Jul 15, 2023

View reviewed changes

damithc approved these changes Jul 19, 2023

View reviewed changes

damithc merged commit a976ec9 into se-edu:master Jul 19, 2023
15 checks passed

	public static String getDuplicatePrefixesToMessage(Set<Prefix> duplicatePrefixes) {
	public static String getDuplicatePrefixesToMessage(Prefix... duplicatePrefixes) {

Removal of repeatable parameters #190

Removal of repeatable parameters #190

Conversation

Eclipse-Dominator commented Jun 6, 2023 • edited by damithc Loading

canihasreview bot commented Jun 6, 2023

codecov bot commented Jun 6, 2023 • edited Loading

Codecov Report

canihasreview bot commented Jun 7, 2023

v1

canihasreview bot commented Jun 7, 2023

v1

damithc commented Jun 7, 2023

damithc commented Jun 8, 2023

damithc left a comment

Choose a reason for hiding this comment

damithc commented Jun 12, 2023

damithc left a comment

Choose a reason for hiding this comment

canihasreview bot commented Jun 12, 2023

v2

jundatan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canihasreview bot commented Jun 13, 2023

v3

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eclipse-Dominator Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eclipse-Dominator Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canihasreview bot commented Jun 14, 2023

v4

canihasreview bot commented Jun 14, 2023

v5

canihasreview bot commented Jun 14, 2023

v6

damithc left a comment

Choose a reason for hiding this comment

canihasreview bot commented Jul 3, 2023

v10

Eclipse-Dominator commented Jul 3, 2023 • edited Loading

damithc commented Jul 3, 2023

canihasreview bot commented Jul 10, 2023

v11

canihasreview bot commented Jul 13, 2023

v12

damithc left a comment

Choose a reason for hiding this comment

canihasreview bot commented Jul 13, 2023

v13

damithc left a comment

Choose a reason for hiding this comment

damithc commented Jul 13, 2023

dlimyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canihasreview bot commented Jul 14, 2023

v14

damithc left a comment

Choose a reason for hiding this comment

canihasreview bot commented Jul 14, 2023

v15

SPWwj left a comment

Choose a reason for hiding this comment

Eclipse-Dominator commented Jun 6, 2023 •

edited by damithc

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

Eclipse-Dominator Jun 14, 2023 •

edited

Loading

Eclipse-Dominator Jun 14, 2023 •

edited

Loading

Eclipse-Dominator commented Jul 3, 2023 •

edited

Loading