ArgumentTokenizer: ensure tokenizing before getting values #312 #320

PierceAndy · 2017-03-06T09:35:46Z

Closes #312.

ArgumentTokenizer's tokenized argument getter methods can be accessed before ArgumentTokenizer is given arguments to tokenize through ArgumentTokenizer#tokenize(String).

Furthermore, the getter methods in ArgumentTokenizer, getValue(Prefix), getAllValues(Prefix), and getPreamble(), don't give a warning when they are called before ArgumentTokenizer#tokenize(...) is ever called.

This behaviour is problematic as the getters can be called before ArgumentTokenizer#tokenize(...) without any warnings, and would return empty optionals, lists, and strings that are indistinguishable from normal empty results.

Link to discussion that highlighted this issue

Proposed merge commit message:

ArgumentTokenizer's tokenized arguments getters can be called before
arguments have been tokenized with ArgumentTokenizer#tokenize(String).

This is problematic as the getters would return empty Optionals, lists,
and strings that are indistinguishable from normal empty results.

Let's encapsulate ArgumentTokenizer's getters in ArgumentMultimap such
that we can enforce the sequence of method calls and simplify
ArgumentTokenizer's logic.

mention-bot · 2017-03-06T09:35:47Z

@PierceAndy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ndt93, @MightyCupcakes and @chao1995 to be potential reviewers.

CanIHasReview-bot · 2017-03-06T09:36:29Z

v1

@PierceAndy submitted v1 for review.

[v1 1/3] ArgumentTokenizer: move tokenize(...) into constructor
[v1 2/3] ArgumentTokenizer: remove unnecessary resetTokenizerState()
[v1 3/3] ArgumentTokenizer: simplify getPreamble() logic

(📚 Archive)

Checkout this PR version locally

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

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

MightyCupcakes

I suppose this solution is chosen over the step-builder pattern because we create a new tokenizer object whenever we parse a user input (and it seems there are currently no use-cases that requires multiple parsing of similar arg strings except in the test cases)

Anyway, looks okay to me.

PierceAndy · 2017-03-07T06:53:05Z

@MightyCupcakes Yes, the step builder pattern and nested classes work, but I realized that they were unnecessarily complex, for the reason you mentioned. I will improve the first commit's message with what you wrote in the next iteration.

Moving the logic into the constructor is preferred over other potential
solutions such as the step builder pattern or nested classes, as this
solution does not involve the creation of additional objects and is more
efficient. Furthermore, there is no need for a more complex solution that
preserves ArgumentTokenizer#tokenize(...) as a separate method, as a
new ArgumentTokenizer object is created each time we parse a user input,
and there are no cases that require consecutive parsing of arguments
with identical prefixes other than in the test cases.

damithc · 2017-03-07T07:25:45Z

@PierceAndy what about this alternative?

Arguments arguments = new ArgumentTokenizer().tokenize(...);
String name = agruments.getValue(...)

There is a general consensus that doing work inside a constructor is not good. But as you said, we don't want to complicate things too much too.

PierceAndy · 2017-03-07T11:07:16Z

@damithc Right, that would be a simple solution while reducing constructor complexity.

CanIHasReview-bot · 2017-03-07T14:23:15Z

v2

@PierceAndy submitted v2 for review.

[v2 1/1] ArgumentTokenizer: extract getters into Arguments class

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

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

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

chao1995

[v2 1/1]

I really like this API design. Good API enforces people to do the right thing.

Some changes suggested below.

chao1995 · 2017-03-08T08:54:55Z

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

-            this.tokenizedArguments.get(prefix).add(value);
+    private void saveArgument(Map<Prefix, List<String>> tokenizedArguments, Prefix prefix, String value) {
+        if (tokenizedArguments.containsKey(prefix)) {
+            tokenizedArguments.get(prefix).add(value);


There is a nice method, getOrDefault, which I think can help to get rid of this method.

Something like

List<String> values = tokenizedArguments.getOrDefault(prefix, new ArrayList<>()); values.add(value); tokenizedArguments.put(prefix, values);

And probably we can just do this inside extractArugments(...).

chao1995 · 2017-03-08T08:59:21Z

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

-    private void resetTokenizerState() {
-        this.tokenizedArguments.clear();
+        Map<Prefix, List<String>> tokenizedArguments = extractArguments(argsString, positions);
+        return new Arguments(tokenizedArguments);


Is it better for extractArguments(...) to return Arguments? Sounds more intuitively. Like

Arguments arguments = extractArguments(argsString, positions); return arguments;

And then only deal with the Map inside extractArguments(...)

Yeah I was debating on which return type to give extractArguments(...).
@MightyCupcakes Thoughts?

Yeah, I agree with @chao1995

chao1995 · 2017-03-08T09:00:42Z

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

     */
-    private void extractArguments(String argsString, List<PrefixPosition> prefixPositions) {
+    private  Map<Prefix, List<String>> extractArguments(String argsString, List<PrefixPosition> prefixPositions) {


So as mentioned, instead of return the map, return Arguments directly.

Note to self: extra space before Map

chao1995 · 2017-03-08T09:04:32Z

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

        for (int i = 0; i < prefixPositions.size() - 1; i++) {
            String argValue = extractArgumentValue(argsString, prefixPositions.get(i), prefixPositions.get(i + 1));
-            saveArgument(prefixPositions.get(i).getPrefix(), argValue);
+            saveArgument(tokenizedArguments, prefixPositions.get(i).getPrefix(), argValue);


As mentioned below, if use getOrDefault, can replace this method with 3 lines of code.

The reason not to do this maybe is for clarity concern, but since now the Map is a local variable, adding an instance method for just saving something to a local map, doesn't feel quite right.

I would argue that saveArgument(...) is following SLAP, and extracting it out into the body of the loop makes the code harder to read and manage. Yes, I would agree that extracting the method back into the loop reduces clarity.

Yes. I didn't use the SLAP term, but I mean it

The reason not to do this maybe is for clarity concern

My reason is that this instance method needs a reference to a local variable (the Map) from another method, I don't feel right doing that. Plus, in my opinion adding a comment is clear enough for this piece of code (just adding a value to a map).

Right, that makes sense. Because you want to avoid mutating something that you're passing by reference.

MightyCupcakes

No further comments

CanIHasReview-bot · 2017-03-08T14:43:25Z

v3

@PierceAndy submitted v3 for review.

[v3 1/1] ArgumentTokenizer: extract getters into Arguments class

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

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

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

CanIHasReview-bot · 2017-03-26T08:58:47Z

v22

@PierceAndy submitted v22 for review.

[v22 1/4] Extract Prefix class out of ArgumentTokenizer class
[v22 2/4] ArgumentTokenizer: extract getters into ArgumentMultimap class
[v22 3/4] Update Logic class diagram with ArgumentMultimap class
[v22 4/4] ArgumentTokenizer: make tokenize(...) static

(📚 Archive) (📈 Interdiff between v21 and v22)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/320/22/head:BRANCHNAME

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

PierceAndy · 2017-03-26T08:58:54Z

v22

Rebase with master

damithc

Minor nits. Otherwise looks good.

damithc · 2017-03-26T10:09:06Z

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

+     */
+    public ArgumentMultimap() {
+        argMultimap = new HashMap<>();
+    }


Can/should we avoid this constructor by initializing the map in line 19?

damithc · 2017-03-26T10:20:37Z

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

+     */
+    public String getPreamble() {
+        Optional<String> storedPreamble = getValue(new Prefix(""));
+        return storedPreamble.orElse("");


Can merge these two lines?

CanIHasReview-bot · 2017-03-26T10:52:46Z

v23

@PierceAndy submitted v23 for review.

[v23 1/4] Extract Prefix class out of ArgumentTokenizer class
[v23 2/4] ArgumentTokenizer: extract getters into ArgumentMultimap class
[v23 3/4] Update Logic class diagram with ArgumentMultimap class
[v23 4/4] ArgumentTokenizer: make tokenize(...) static

(📚 Archive) (📈 Interdiff between v22 and v23)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/320/23/head:BRANCHNAME

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

PierceAndy · 2017-03-26T10:53:21Z

v23

Minor improvements to ArgumentMultimap class

PierceAndy · 2017-03-26T10:55:25Z

@MightyCupcakes A couple of minor changes since your last review.

CanIHasReview-bot · 2017-03-27T07:20:32Z

v24

@PierceAndy submitted v24 for review.

[v24 1/4] Extract Prefix class out of ArgumentTokenizer class
[v24 2/4] ArgumentTokenizer: extract getters into ArgumentMultimap class
[v24 3/4] Update Logic class diagram with ArgumentMultimap class
[v24 4/4] ArgumentTokenizer: make tokenize(...) static

(📚 Archive) (📈 Interdiff between v23 and v24)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/320/24/head:BRANCHNAME

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

PierceAndy · 2017-03-27T07:20:51Z

v24

Fix minor typo.

MightyCupcakes

[v24 3/4] Commit organisation.

MightyCupcakes · 2017-03-27T09:16:26Z

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

-    public ArgumentMultimap() {
-        argMultimap = new HashMap<>();
-    }
+    private final Map<Prefix, List<String>> argMultimap = new HashMap<>();


Should be in commit 2 instead of this commit

ArgumentTokenizer's tokenized argument getters can be accessed even before ArgumentTokenizer is given arguments to tokenize through ArgumentTokenizer#tokenize(String). This behaviour is problematic as the getters can be called before ArgumentTokenizer#tokenize(...), and would return empty optionals, lists, and strings that are indistinguishable from normal empty results. Let's extract the getters into a new ArgumentMultimap class, and have ArgumentTokenizer#tokenize(...) return an instance of that class so that these getters cannot be accessed before tokenization.

The Logic class diagram in the developer guide does not reflect the updated dependencies between ArgumentTokenizer, ArgumentMultimap, Prefix, and command parser classes. Let's update the Logic class diagram.

Command parsers need to create a new ArgumentTokenizer object before they can tokenize an arguments string. During instantiation, the ArgumentTokenizer constructor is provided prefixes to be later used when ArgumentTokenizer#tokenize(String) is called. ArgumentTokenizer doesn't have a need to be instantiated before tokenizing can be done. The object is not reused to tokenize different arguments strings with the same set of prefixes provided during instantiation. Let's make ArgumentTokenizer#tokenize(...) static, which also makes it state-independent and thus easier to test.

CanIHasReview-bot · 2017-03-27T09:27:25Z

v25

@PierceAndy submitted v25 for review.

[v25 1/4] Extract Prefix class out of ArgumentTokenizer class
[v25 2/4] ArgumentTokenizer: extract getters into ArgumentMultimap class
[v25 3/4] Update Logic class diagram with ArgumentMultimap class
[v25 4/4] ArgumentTokenizer: make tokenize(...) static

(📚 Archive) (📈 Interdiff between v24 and v25)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/320/25/head:BRANCHNAME

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

PierceAndy · 2017-03-27T09:29:04Z

v25

Move erroneous changes from [v24 3/4] to [v24 2/4]

CanIHasReview-bot added the s.ToReview label Mar 6, 2017

PierceAndy requested review from MightyCupcakes and chao1995 March 6, 2017 09:36

MightyCupcakes reviewed Mar 7, 2017

View reviewed changes

PierceAndy added s.Ongoing and removed s.ToReview labels Mar 7, 2017

PierceAndy force-pushed the 312-ensure-tokenize-before-get branch from 9a53ec0 to 8e39586 Compare March 7, 2017 14:22

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 7, 2017

PierceAndy requested a review from MightyCupcakes March 7, 2017 14:24

chao1995 suggested changes Mar 8, 2017

View reviewed changes

MightyCupcakes reviewed Mar 8, 2017

View reviewed changes

MightyCupcakes added s.Ongoing and removed s.ToReview labels Mar 8, 2017

PierceAndy force-pushed the 312-ensure-tokenize-before-get branch 3 times, most recently from c43ab13 to a91e196 Compare March 8, 2017 14:23

PierceAndy closed this Mar 8, 2017

PierceAndy force-pushed the 312-ensure-tokenize-before-get branch from a91e196 to c3f8027 Compare March 8, 2017 14:27

PierceAndy reopened this Mar 8, 2017

PierceAndy force-pushed the 312-ensure-tokenize-before-get branch from c5cb5e5 to 0c63ccf Compare March 8, 2017 14:41

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 8, 2017

PierceAndy force-pushed the 312-ensure-tokenize-before-get branch from eaae7e3 to 9c87cb5 Compare March 26, 2017 08:58

damithc suggested changes Mar 26, 2017

View reviewed changes

PierceAndy force-pushed the 312-ensure-tokenize-before-get branch from 9c87cb5 to 77aa2e6 Compare March 26, 2017 10:52

CanIHasReview-bot requested a review from damithc March 26, 2017 10:52

PierceAndy requested a review from MightyCupcakes March 26, 2017 10:54

PierceAndy force-pushed the 312-ensure-tokenize-before-get branch from 77aa2e6 to 31411b9 Compare March 27, 2017 07:20

MightyCupcakes suggested changes Mar 27, 2017

View reviewed changes

PierceAndy added 3 commits March 27, 2017 17:25

Update Logic class diagram with ArgumentMultimap class

0d2e7b2

The Logic class diagram in the developer guide does not reflect the updated dependencies between ArgumentTokenizer, ArgumentMultimap, Prefix, and command parser classes. Let's update the Logic class diagram.

PierceAndy force-pushed the 312-ensure-tokenize-before-get branch from 31411b9 to b3baf26 Compare March 27, 2017 09:26

CanIHasReview-bot requested a review from MightyCupcakes March 27, 2017 09:27

PierceAndy requested a review from chao1995 March 27, 2017 09:29

chao1995 approved these changes Mar 28, 2017

View reviewed changes

MightyCupcakes approved these changes Mar 28, 2017

View reviewed changes

chao1995 mentioned this pull request Mar 28, 2017

Remove TestPerson #310 #335

Closed

damithc approved these changes Mar 28, 2017

View reviewed changes

PierceAndy added s.ToMerge and removed s.ToReview labels Mar 28, 2017

damithc changed the title ~~ArgumentTokenizer: tokenizing is not ensured before getting values #312~~ ArgumentTokenizer: ensure tokenizing before getting values #312 Mar 29, 2017

damithc merged commit 2d93a18 into se-edu:master Mar 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ArgumentTokenizer: ensure tokenizing before getting values #312 #320

ArgumentTokenizer: ensure tokenizing before getting values #312 #320

PierceAndy commented Mar 6, 2017 •

edited

Loading

mention-bot commented Mar 6, 2017

CanIHasReview-bot commented Mar 6, 2017

MightyCupcakes left a comment

PierceAndy commented Mar 7, 2017

damithc commented Mar 7, 2017

PierceAndy commented Mar 7, 2017

CanIHasReview-bot commented Mar 7, 2017

chao1995 left a comment •

edited

Loading

chao1995 Mar 8, 2017

chao1995 Mar 8, 2017

PierceAndy Mar 8, 2017

MightyCupcakes Mar 8, 2017

chao1995 Mar 8, 2017 •

edited

Loading

PierceAndy Mar 8, 2017

chao1995 Mar 8, 2017 •

edited

Loading

PierceAndy Mar 8, 2017 •

edited

Loading

chao1995 Mar 8, 2017 •

edited

Loading

PierceAndy Mar 8, 2017

MightyCupcakes left a comment

CanIHasReview-bot commented Mar 8, 2017

CanIHasReview-bot commented Mar 26, 2017

PierceAndy commented Mar 26, 2017

damithc left a comment

damithc Mar 26, 2017

damithc Mar 26, 2017

CanIHasReview-bot commented Mar 26, 2017

PierceAndy commented Mar 26, 2017

PierceAndy commented Mar 26, 2017

CanIHasReview-bot commented Mar 27, 2017

PierceAndy commented Mar 27, 2017

MightyCupcakes left a comment

MightyCupcakes Mar 27, 2017

CanIHasReview-bot commented Mar 27, 2017

PierceAndy commented Mar 27, 2017

ArgumentTokenizer: ensure tokenizing before getting values #312 #320

ArgumentTokenizer: ensure tokenizing before getting values #312 #320

Conversation

PierceAndy commented Mar 6, 2017 • edited Loading

mention-bot commented Mar 6, 2017

CanIHasReview-bot commented Mar 6, 2017

v1

MightyCupcakes left a comment

Choose a reason for hiding this comment

PierceAndy commented Mar 7, 2017

damithc commented Mar 7, 2017

PierceAndy commented Mar 7, 2017

CanIHasReview-bot commented Mar 7, 2017

v2

chao1995 left a comment • 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

chao1995 Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chao1995 Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

PierceAndy Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

chao1995 Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MightyCupcakes left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 8, 2017

v3

CanIHasReview-bot commented Mar 26, 2017

v22

PierceAndy commented Mar 26, 2017

v22

damithc 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 Mar 26, 2017

v23

PierceAndy commented Mar 26, 2017

v23

PierceAndy commented Mar 26, 2017

CanIHasReview-bot commented Mar 27, 2017

v24

PierceAndy commented Mar 27, 2017

v24

MightyCupcakes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 27, 2017

v25

PierceAndy commented Mar 27, 2017

v25

PierceAndy commented Mar 6, 2017 •

edited

Loading

chao1995 left a comment •

edited

Loading

chao1995 Mar 8, 2017 •

edited

Loading

chao1995 Mar 8, 2017 •

edited

Loading

PierceAndy Mar 8, 2017 •

edited

Loading

chao1995 Mar 8, 2017 •

edited

Loading