[CS2113T-T13-1] Shoco #15

JLoh579 · 2020-03-03T05:07:02Z

@kokjoon97 @Shannonwje @jiajuinphoon @trishaangelica

Fix stock commands to be directly executed.

yantingsanity

Hello! I think that your team's DG looks quite good, just that there are some minor fixes here and there! 👍 Perhaps, you guys can check if you are using Duke class or Shoco class as there are some confusions in some of the diagrams and maybe can think of more design considerations too? Overall, good work! 😄

yantingsanity · 2020-03-30T08:15:57Z

docs/DeveloperGuide.md

+ The following sequence diagram below shows how the add feature works. The details of the adding item's values
+ are shown in a separate sequence diagram below:
+
+ ![alt text](images/AddFeature.png)


Is the activation bar for the Duke supposed to be longer? as I assumed that the Duke instance will still be there while AddCommand is executing its commands. 😆

The sequence diagram is clear on how it works. I also think that the activation bar should be longer. Other than that, It looks great

yantingsanity · 2020-03-30T08:25:50Z

docs/DeveloperGuide.md

+
+ ![alt text](images/AddFeature.png)
+
+ ![alt text](images/AddFeature_SD.png)


As per the image below,

Should the method name be the same as the ref frame which stated "Add item values"?

Should the alt frame be more near the left corner? 😆

I think that this "X" in this frame may be a little redundant as it was the "X" can be found in your above diagram for the AddCommand instance.

yantingsanity · 2020-03-30T08:33:29Z

docs/DeveloperGuide.md

+1. <code>Duke</code> class receives user input from the <code>Ui</code> class. 
+2. A <code>Parser</code> object is created to call its <code>parseCommand</code> method.
+* The <code>Parser</code> object instantiates an <code>EditCommand</code> object based on the user input.
+3. The <code>Duke</code> class calls the <code>execute</code> method of the <code>EditCommand</code> object.


Perhaps for the execute method, you can call it as EditCommand#execute()?

yantingsanity · 2020-03-30T08:41:31Z

docs/DeveloperGuide.md

+2. A <code>Parser</code> object is created to call its <code>parseCommand</code> method.
+* The <code>Parser</code> object instantiates an <code>EditCommand</code> object based on the user input.
+3. The <code>Duke</code> class calls the <code>execute</code> method of the <code>EditCommand</code> object.
+4. In the <code>execute</code> function, the <code>item</code> to be edited (based on the specified index of the 


Perhaps, you can state what function you are calling to get the specified index as seen in your diagram for more user readability?

thanks for that suggestion, I will include it in!

yantingsanity · 2020-03-30T08:45:47Z

docs/DeveloperGuide.md

+
+- Alternative 1 (current choice): User must provided a description for item, Duplicates are
+                                  not allowed in the list. 
+  - Pros: User has minimal potential to see unreasonable list in the Shopping List. For 


Could this section be more simplified and maybe more explanation? It was a little confusing 😆
Maybe, can explain more about what is unreasonable list and minimal potential so that users can understand.
Perhaps could use words like "lesser probability of viewing incorrectly formatted list?"

yantingsanity · 2020-03-30T08:46:42Z

docs/DeveloperGuide.md

+
+##### Aspect: Data structure to support the edit feature
+
+- Alternative 1 (current choice): Only parameters present in user input are treated as values to update.


Good design considerations! 👍

yantingsanity · 2020-03-30T08:50:07Z

docs/DeveloperGuide.md

+<code>Command</code> class with a String representing the keyword specified by the user.
+
+The <code>Shoco</code> class first receives user input from the <code>Ui</code> class before it creates a 
+<code>Parser</code> object and calls its <code>parseCommand</code> function to instantiate a 


Perhaps, you can use Parser#parseCommand() and for the other methods in your description?

yantingsanity · 2020-03-30T08:54:18Z

docs/DeveloperGuide.md

+The following sequence diagram below shows how the clear list feature works. Note the <code>Ui</code> class is
+omitted to emphasise the other classes:
+
+![alt text](images/Clear.png)


Is this supposed to be Duke class or Shoco class? haha

yantingsanity · 2020-03-30T08:55:38Z

docs/DeveloperGuide.md

+The following sequence diagram below shows how the reset budget feature works. Note the <code>Ui</code> class is
+omitted in the sequence diagram to emphasise on the other classes:
+
+![alt text](images/Reset_Budget.png)


Can refer to the comment in Clear List feature!

lowxizhi · 2020-03-31T04:39:32Z

3.1.1 Current implementation, Process of object creation:

The reference frame names "add item values" vs "update item values" are not consistent.

lowxizhi · 2020-03-31T04:40:40Z

3.1.1 Current implementation, Process of object creation:

Duke class is not mentioned previously in DG. Is it supposed to be Shoco class?

lowxizhi · 2020-03-31T04:42:17Z

3.1.1 Current implementation, Process of object creation:

Activation bar for constructors in Parser and AddCommand are not attached to the head.

synCKun

DG review, good job overall!

synCKun · 2020-03-31T04:30:26Z

docs/DeveloperGuide.md

+
+##### Aspect: Data structure to support the add feature
+
+- Alternative 1 (current choice): User must provided a description for item, Duplicates are


Maybe this section could be clearer on why alternative 1 was chosen over alternative 2.

synCKun · 2020-03-31T04:34:39Z

docs/DeveloperGuide.md

+The following sequence diagram below shows how the edit feature works. The details of updating the items' values
+have been omitted from the diagram. Those details are shown in a separate sequence diagram below:
+
+![alt text](images/EditFeature.png)


I was wondering if the activation bar of duke can just be replaced by a life line, since the activation bar ends before those of the called functions, which may be confusing.

synCKun · 2020-03-31T04:36:49Z

docs/DeveloperGuide.md

+![alt text](images/EditFeature_SD.jpg)
+
+
+#### 3.2.2 Design considerations


Good considerations, could be clearer if a short line were to be given to explain why the current choice is chosen over the other alternative.

synCKun · 2020-03-31T04:43:42Z

docs/DeveloperGuide.md

+ 3. The <code>Duke</code> class calls the <code>execute</code> method of the <code>AddCommand</code> object.
+ 4. In the <code>execute</code> function, the <code>item</code> to be add is called from the <code>ShoppingList</code> 
+    object, using items.add().
+ 5. In the SD, the AddCommand will add <code>item</code> if the description is provided and one / both price and 


What does SD stand for? :o

lowxizhi · 2020-03-31T04:51:34Z

3.1.1 Current implementation, Process of object creation, Point 4:

From this statement "the item to be add is called from the ShoppingList object, using items.add()", I infer that items.add() calls the item to be added from the ShoppingList object. This seems problematic as:

The meaning of "add" does not corresponds with the description "to call the item to be added". Consider changing the name of the method to fit its purpose better, or rephrase the purpose of items.add() in the description.
In the sequence diagram, the arrow labelled item.add(add) was pointing from the AddCommand class to the ShoppingList class, which does not correspond to the statement of "the item to be added is called from the ShoppingList object". Consider rephrasing the purpose of items.add() in the description.
In the sequence diagram, the method item.add(add) is called. However, there is no indication or explanation for the variable "add" that was passed into the method item.add().
On the overall, from the description and sequence diagram, I find it confusing as to where the item was added, and where the item to be added came from.

lowxizhi · 2020-03-31T04:56:02Z

3.1.1 Current implementation, Process of object creation:

In the sequence diagram, the object name "items"/"Items" is not consistent in "Items.add(add)" method and "items:ShoppingList".

lowxizhi · 2020-03-31T04:57:22Z

3.1.1 Current implementation, Process of object creation:

In the sequence diagram, the alt tab is not attached to the top left corner of the box.

lowxizhi · 2020-03-31T05:00:06Z

In the design considerations of each implemented feature, there is good explanation of the pros and cons of the alternatives considered. It would be good if the reason for ultimately choosing 1 alternative over the other is stated.

lowxizhi · 2020-03-31T05:03:57Z

UML diagrams are generally neat and consistent throughout the DG. One mistake spotted throughout is that the activation bar for constructors are not attached to the head.

nguan1

Overall, a clear and concise developer guide that is well formatted with clear diagrams.

For the design considerations section for your commands, it may be prudent to make a single statement as to why you chose to create a separate command class for each command, instead of repeating similar reasoning for each command.

Also, the activation bar should attached to the class, when you call a constructor, in your sequence diagrams.

For example, you guys have the bars detached from the class when Parser() is called. However, the module requires you to do something like this:

Otherwise your group's developer guide is a quality piece of work.
Keep up the great work!

rsanchez-macias

Great DG! I really liked your sequence diagrams and the features you added. The pros and cons were nice to have an idea on how you approached the project. 👍

rsanchez-macias · 2020-03-31T06:13:09Z

docs/DeveloperGuide.md

+ <code>Command</code> class. The user input **must contain at least a description** out of these parameters: 
+ *description*, *price*, *quantity*. User can choose not to input price or quantity as the price will set to 
+ default which is 0.0 if the user did not input any value for price. On the other hand, quantity will set to 
+ default which is 1 if the user did not input any value for quantity. 


The description for the add feature is great, maybe adding a sample input and output would help see how the feature works?

rsanchez-macias · 2020-03-31T06:17:52Z

docs/DeveloperGuide.md

+ The following sequence diagram below shows how the add feature works. The details of the adding item's values
+ are shown in a separate sequence diagram below:
+
+ ![alt text](images/AddFeature.png)


The sequence diagram is clear on how it works. I also think that the activation bar should be longer. Other than that, It looks great

rsanchez-macias · 2020-03-31T06:19:35Z

docs/DeveloperGuide.md

+
+ ![alt text](images/AddFeature_SD.png)
+
+#### 2.1.2 Design considerations


Just a minor thing, should this section be 3.1.2?

rsanchez-macias · 2020-03-31T06:31:49Z

docs/DeveloperGuide.md

+
+Diagram 2:
+
+![alt text](images/Unmark.png)


Should the activation bar UnmarkCommand be longer than ShoppingList? I am thinking that control returns to UnmarkCommand after an item is unmarked in ShoppingList

rsanchez-macias · 2020-03-31T06:33:17Z

docs/DeveloperGuide.md

+
+   - Cons: Might significantly increase the code base with another class being added
+
+- Alternative 2: Implement the mark and unmark feature in either the <code>Duke</code> or <code>Parser</code> class


Maybe rename Duke to Shoco to match the name of the app?

Other than that, great explanations 👍

rsanchez-macias · 2020-03-31T06:44:03Z

docs/DeveloperGuide.md

+The following sequence diagram below shows how the <code>Shoco</code> object creates the <code>FindCommand</code> object. Note the <code>Ui</code> class is
+omitted in the sequence diagram to emphasise on the other classes:
+
+![alt text](images/Find.png)


The sequence diagrams are great, and I have an idea of how the app works now. However, I noticed that most sequence diagrams contain how Duke and Parser work together, and the only things that change are the circled parts (FindCommand, AddCommand, DisplayCommand). Maybe you could add a separate section on how Duke and Parser work together, and then omit it from the other sequence diagrams?

Then, the diagrams would be more focused on the feature itself

rsanchez-macias · 2020-03-31T06:47:09Z

docs/DeveloperGuide.md

+having to edit the mark and unmark feature difficult.
+
+&nbsp;
+<b><a href="#developer-guide">&#129053; back to top</a></b>


I really like that you added links to go back to the top :) very useful 👍

rsanchez-macias · 2020-03-31T06:52:59Z

docs/DeveloperGuide.md

+The reset budget feature is implemented using a <code>ResetBudgetCommand</code> class which extends the main
+<code>Command</code> class with a variable representing the budget amount.
+
+The <code>Duke</code> class first receives user input from the <code>Ui</code> class before it creates a 


For section 3 in general (implementation), the explanations for each feature are good. I noticed that for the add feature, the implementation is explained in a list, but it is just paragraphs for some other features. I think that maybe doing all of the explanations like the add feature would be great, and then it would help with consistency.

Oh sorry, there are two screenshots, one for AddCommand and another for DisplayCommand. White background makes it look like there is only one 😅

rsanchez-macias · 2020-03-31T06:56:48Z

docs/DeveloperGuide.md

+### Purpose of this guide
+This guide describes the software architecture and design of the SHOCO application.
+It will evolve throughout the design and implementation of each SHOCO release. 
+Currently, this document is for the first public release of the application, SHOCO v1.0.


Small thing, but should this be SHOCO v2.0? I saw that the Appendix contains info about v2.0

RenzoTsai

Very good DG. it is concise and clear!

RenzoTsai · 2020-03-31T07:02:00Z

docs/DeveloperGuide.md

+class, <code>WriteData</code> class, <code>FileUtil</code> class and
+<code>CommandLineTable</code> class.
+
+![alt text](images/ClassDiag.png)


Maybe here lacks of the some Associations. e.g : In your Parser, there is a private newCommand, so there should be one link between Parser and Command.

RenzoTsai · 2020-03-31T07:09:37Z

docs/DeveloperGuide.md

+
+<!-- @@author kokjoon97 -->
+The <code>Shoco</code> class manages all required resources in the execution of the application. These include
+a <code>ShoppingList</code> object to keep track of the <code>Item</code> objects the user has added to his list and
+a <code>Budget</code> object to store the user's budget.
+
+<code>Shoco</code> also has a <code>Storage</code> object for saving and loading data from memory - this data includes
+the latest saved <code>ShoppingList</code> and <code>Budget</code>.
+
+There is a dependency from <code>Shoco</code> to <code>Parser</code> as it only creates an instance of the <code>Parser</code>
+every time user input is received by the <code>Ui</code> and does not keep track of the <code>Parser</code> which is deleted
+after it is done parsing the current user input. The <code>Parser</code> determines what command is being invoked by the
+user before creating a new <code>Command</code> object. It then returns the reference to the new <code>Command</code> object 
+to <code>Shoco</code>. 
+
+At any point in time, <code>Shoco</code> only stores up to one <code>Command</code> and no more. This
+<code>Command</code> has to be executed before <code>Shoco</code> can receive more user input.
+<!-- @@author -->
+&nbsp;
+
+<b><a href="#developer-guide">&#129053; back to top</a></b>
+&nbsp;


Very clear overview. Looks good!

RenzoTsai · 2020-03-31T07:14:00Z

docs/DeveloperGuide.md

+ The following sequence diagram below shows how the add feature works. The details of the adding item's values
+ are shown in a separate sequence diagram below:
+
+ ![alt text](images/AddFeature.png)


Is it supposed to add an object creation mark here?

RenzoTsai · 2020-03-31T07:24:53Z

docs/DeveloperGuide.md

+
+This next sequence diagram will show how the <code>FindCommand</code> creates the <code>filteredItems</code> list:
+
+![alt text](images/FindItem.png)


Is it supposed to add activation bars in this picture?

JeremyTakigawa · 2020-03-31T07:32:28Z

src/main/java/seedu/duke/commands/FindCommand.java

@@ -0,0 +1,70 @@
+//@@author kokjoon97


The sequence diagram used here is using minimal notation, but other diagrams have activiation bars so probably it is better to keep everything the same style.

JeremyTakigawa · 2020-03-31T08:00:52Z

src/main/java/seedu/duke/utils/Parser.java

@@ -0,0 +1,682 @@
+package seedu.duke.utils;


I think normal it is more like normal association so maybe change the dashed line in the UML?

JeremyTakigawa · 2020-03-31T08:14:40Z

src/main/java/seedu/duke/commands/Command.java

@@ -0,0 +1,27 @@
+package seedu.duke.commands;


It should be better to let Shoco class has 1 Command class everytime as 0..1 is a bit weird.

JeremyTakigawa

Overall it is a very well-written DG.

Branch portfolio

# Conflicts: # docs/DeveloperGuide.md

# Conflicts: # docs/DeveloperGuide.md # docs/slides/MarkUnmark.pptx # docs/slides/ResetBudget.pptx

Branch resize images

Branch refine v2.1

…into branch-PPPUpdate

Branch ppp update

Resize images....

…into branch-PPPUpdate

Branch ppp update

Resize images again

Final resize of images

Resize of images, edit pr issue number

Branch developer guide

Branch portfolio

Fix issue with diagrams not appearing

damithc pushed a commit to damithc/tp-draft that referenced this pull request Mar 14, 2020

Merge pull request nus-cs2113-AY1920S2#15 from JosephLimWeiJie/master

17022f3

Fix stock commands to be directly executed.

yantingsanity reviewed Mar 30, 2020

View reviewed changes

synCKun reviewed Mar 31, 2020

View reviewed changes

nguan1 reviewed Mar 31, 2020

View reviewed changes

rsanchez-macias reviewed Mar 31, 2020

View reviewed changes

RenzoTsai reviewed Mar 31, 2020

View reviewed changes

JeremyTakigawa reviewed Mar 31, 2020

View reviewed changes

jiajuinphoon and others added 14 commits April 3, 2020 22:35

Merge pull request #186 from jiajuinphoon/branch-portfolio

3e1ab12

Branch portfolio

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T13-1/tp

4918346

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T13-1/tp

fae3596

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T13-1/tp

4bd4bc5

Merge branch 'master' into branch-DeveloperGuide

eb76761

Merge branch 'master' into branch-UserGuide

82d80fd

# Conflicts: # docs/DeveloperGuide.md

Edited sequence diagrams for mark, unmark and reset budget feature

b8d0a68

Merge branch 'master' into branch-UserGuide

bb92c4e

# Conflicts: # docs/DeveloperGuide.md # docs/slides/MarkUnmark.pptx # docs/slides/ResetBudget.pptx

Edited size of pics

c09db8e

Merge branch 'master' into branch-UserGuide

0d94b0f

Resize images

a447bc2

Merge pull request #187 from trishaangelica/Branch-ResizeImages

035d9e4

Branch resize images

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T13-1/tp

2f89b50

date and time in DG

8d59715

JLoh579 and others added 30 commits April 11, 2020 13:38

Add and edit some Javadocs

4122a0d

Resize images

b25c004

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T13-1/tp

d8d26d7

Merge branch 'master' into branch-PPPUpdate

3d83d08

Correct checkstyle violations

21e90a4

no message

96ff8bb

Merge pull request #231 from JLoh579/branch-refineV2.1

1d4511d

Branch refine v2.1

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T13-1/tp …

e51f24b

…into branch-PPPUpdate

Merge pull request #232 from trishaangelica/branch-PPPUpdate

63dc78f

Branch ppp update

Resize images....

1610484

Merge pull request #233 from trishaangelica/branch-PPPUpdate

d379c92

Resize images....

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T13-1/tp …

c149bc0

…into branch-PPPUpdate

Resize images

1167dff

Merge pull request #234 from trishaangelica/branch-PPPUpdate

54daca3

Branch ppp update

Resize images again

fb3160c

Merge pull request #235 from trishaangelica/branch-PPPUpdate

9661803

Resize images again

Final resize of images

e993866

Merge pull request #236 from trishaangelica/branch-PPPUpdate

beb8498

Final resize of images

Resize of images, edit issue number

01685cb

Merge pull request #237 from trishaangelica/branch-PPPUpdate

3ca729c

Resize of images, edit pr issue number

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T13-1/tp

3362af1

Merge branch 'master' into branch-portfolio

3a36458

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T13-1/tp

afd9b81

Merge branch 'master' into branch-DeveloperGuide

10165e9

Resized images for DG

5e4dcd1

Merge pull request #238 from Shannonwje/branch-DeveloperGuide

6f3a614

Branch developer guide

Refine PPP

78c656b

Merge pull request #239 from JLoh579/branch-portfolio

33a65b2

Branch portfolio

Fix issue with diagrams not appearing

12fc85c

Merge pull request #240 from JLoh579/branch-portfolio

8d10f60

Fix issue with diagrams not appearing


		![alt text](images/AddFeature.png)

		![alt text](images/AddFeature_SD.png)


		##### Aspect: Data structure to support the edit feature

		- Alternative 1 (current choice): Only parameters present in user input are treated as values to update.


		##### Aspect: Data structure to support the add feature

		- Alternative 1 (current choice): User must provided a description for item, Duplicates are

		![alt text](images/EditFeature_SD.jpg)


		#### 3.2.2 Design considerations


		![alt text](images/AddFeature_SD.png)

		#### 2.1.2 Design considerations


		- Cons: Might significantly increase the code base with another class being added

		- Alternative 2: Implement the mark and unmark feature in either the <code>Duke</code> or <code>Parser</code> class


		This next sequence diagram will show how the <code>FindCommand</code> creates the <code>filteredItems</code> list:

		![alt text](images/FindItem.png)

[CS2113T-T13-1] Shoco #15

Are you sure you want to change the base?

[CS2113T-T13-1] Shoco #15

Conversation

JLoh579 commented Mar 3, 2020 • edited Loading

yantingsanity left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowxizhi commented Mar 31, 2020

lowxizhi commented Mar 31, 2020

lowxizhi commented Mar 31, 2020

synCKun left a 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

Choose a reason for hiding this comment

lowxizhi commented Mar 31, 2020 • edited Loading

lowxizhi commented Mar 31, 2020

lowxizhi commented Mar 31, 2020

lowxizhi commented Mar 31, 2020

lowxizhi commented Mar 31, 2020

nguan1 left a comment

Choose a reason for hiding this comment

rsanchez-macias left a 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

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

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

RenzoTsai left a 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

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

JeremyTakigawa left a comment

Choose a reason for hiding this comment

JLoh579 commented Mar 3, 2020 •

edited

Loading

lowxizhi commented Mar 31, 2020 •

edited

Loading