[CS2113T-T10-1] SplitLah by froststein · Pull Request #8 · nus-cs2113-AY2122S2/tp

froststein · 2022-03-03T02:14:06Z

SplitLah is an application for keeping track of expenses during group outing sessions, specifically for those who are cautious about their finance. It then proceeds to split the expenses according to the group members' individual costs for the activities they participated in, for a particular session, via a Command Line Interface(CLI).

BradenTeo

Overall, I think many diagrams can be simplified and resized to be bigger so that they are visible in PDF form.

BradenTeo · 2022-03-31T01:21:16Z

+
+## Design
+### Architecture
+![Application Diagram Screenshot](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/ArchitectureDiagram.drawio.png)


Perhaps you could label the person and the file icons on the left and right.

Is the arrow from manager to textUi correct? Cause the TextUi invokes an instance the manager

BradenTeo · 2022-03-31T01:35:40Z

+    * Defines how a command is executed.
+
+**Interaction between components**
+![Component Interaction Screenshot](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/ComponentInteraction.drawio.png)


I think it might be confusing for the user to see 2 instances of the :SplitLah class here. You could label or explain what do each of them represent. Maybe it's possible to remove one for better comprehendability.

Overall, I think the diagram looks quite small on the website. Perhaps can consider prioritising comprehensibility over being comprehensive.

BradenTeo · 2022-03-31T01:45:31Z

+class would return a unique identifier every time a new `Session`, `Activity` or `Group` is created. 
+
+### TextUI Component
+![TextUI Component Screenshot](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/TextUI%20Component.drawio.png)


The picture here cannot be seen.

laiisaac

Developer guide is generally well done, with comprehensive and correct diagrams to supplement text explanations

laiisaac · 2022-03-31T01:06:38Z

+    * [Storage Component](#storage-component)
+    * [Parser Component](#parser-component)
+    * [Command Component](#command-component)
+* Implementation


Inconsistent formatting: I think there's a missing a hyperlink to the Implementation section

Might want to use numbers instead of bullet points given the comprehensive nature of your guide which makes very hard to follow through

laiisaac · 2022-03-31T01:09:36Z

+
+## Design
+### Architecture
+![Application Diagram Screenshot](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/ArchitectureDiagram.drawio.png)


Is the sequence diagram too detailed? It is hard to see what is being written in the diagram unless it is zoomed into.
What I see at default zoom level:

Perhaps you can consider splitting it up into different parts (e.g. 1 diagram for loading the file, 1 diagram for processing user input).

laiisaac · 2022-03-31T01:14:42Z

+class would return a unique identifier every time a new `Session`, `Activity` or `Group` is created. 
+
+### TextUI Component
+![TextUI Component Screenshot](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/TextUI%20Component.drawio.png)


Screenshot for TextUI component does not load:

laiisaac · 2022-03-31T01:19:17Z

+1. After a `XYZCommand` object is created by the [`Parser` Component](#parser-component),
+   it is passed back to the `SplitLah` object to be run.
+   * In the above process, all necessary information for the execution of the command is passed to and
+     saved by the `XYZCommand` constructor.
+2. Then, `XYZCommand#run` is executed by `SplitLah`. `XYZCommand#run` carries out the task
+   that `XYZCommand` is designed to do.
+   * In general, for _data related commands_, `SessionJKLCommand`, `ActivityJKLCommand` and `GroupJKLCommand`
+     obtains the relevant `Session`, `Activity` and `Group` objects before operating upon them with relevant methods.
+   * On the other hand, for _utility commands_, `UtilityCommand` works with and uses methods from the `TextUI` component
+     to print messages and carry out their tasks.
+   * The inner workings of each of the `XYZCommand` classes can be seen in greater detail under the [Implementation section](#implementation).


Good job on the formatting as well as clarification on certain ambiguous terms used, makes it very readable for the reader

laiisaac · 2022-03-31T01:26:52Z

+   ![Reference Frame Command Parser Sequence Diagram](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/RefCommandParser.drawio.png)

-## Design & implementation
+   ![Reference Frame ParseABC Sequence Diagram](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/RefParseABC.drawio.png)

-{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
+   ![Reference Frame InvalidCommand Instantiation Sequence Diagram](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/RefInvalidCommand.drawio.png)


Diagrams provided are not explicitly linked to text explaining it. Perhaps you can consider explaining each diagram with text right below the diagram, or name the diagrams and refer to them explicitly when explaining the respective process in text.

laiisaac · 2022-03-31T01:36:54Z

+   and finally back to `SplitLah` to be run.
+
+### Add a session
+**API reference:** [`SessionCreateCommand.java`](https://github.com/AY2122S2-CS2113T-T10-1/tp/blob/master/src/main/java/seedu/splitlah/command/SessionCreateCommand.java)


I like that you've coloured classes and objects according to their role:

However, some diagrams such as this one does not follow the same formatting:

Perhaps the document would be more cohesive with consistent formatting.

cczhouqi

Very detailed developer guide. Diagrams are well-designed and clear explanation is given.👍🏻

cczhouqi · 2022-03-31T03:15:14Z

+when the user invokes the `activity /list` command.
+<br>
+<br>
+![List Activity Sequence Diagram Screenshot](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/ActivityListCommand.drawio.png)


This screenshot is also not shown.

cczhouqi · 2022-03-31T03:19:25Z

+   calling `TableFormatter#addRow` for each group to create a table with the summary of each group. A `String` object
+   representing the table is then returned.
+6. The `String` object retrieved is printed out with `TextUI#printlnMessage`.



The diagrams and descriptions are detailed and clear. Good job!👍🏻👍🏻

ibrahimisramos

Great job team!

ibrahimisramos · 2022-03-31T03:23:42Z

+    * [Storage Component](#storage-component)
+    * [Parser Component](#parser-component)
+    * [Command Component](#command-component)
+* Implementation


Might want to use numbers instead of bullet points given the comprehensive nature of your guide which makes very hard to follow through

ibrahimisramos · 2022-03-31T03:24:43Z

+
+## Design
+### Architecture
+![Application Diagram Screenshot](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/ArchitectureDiagram.drawio.png)


Is the arrow from manager to textUi correct? Cause the TextUi invokes an instance the manager

ibrahimisramos · 2022-03-31T03:25:23Z

+10. Finally, with the `TextUI` object, the method `printlnMessageWithDivider` is called to print the message
+    obtained from the `SessionSummaryCommand#processAllTransactions` method.
+
+### Add an activity


Do remember to include them once they are done!

ibrahimisramos · 2022-03-31T03:30:45Z

+| v1.0    | Budget conscious user | list all existing sessions           | view all sessions previously created                                        |
+| v1.0    | Budget conscious user | list all activities in a session     | view all the activities that happened in the session                        |
+| v1.0    | Budget conscious user | settle all transactions of a session | see a summary of who needs to pay what amount to who for the entire session |
+| v1.0    | User                  | exit the application                 | stop tracking                                                               |


Do add in v2.0 and can consider having a separate table for each user type or sort them by different types of user

ibrahimisramos · 2022-03-31T03:31:15Z

-
-{Give non-functional requirements}
+1. The application should be able to work in any operating systems with `Java 11` installed.
+2. 


Otherwise good job! Very detailed but might be a bit too detailed

yuzhongng · 2022-03-31T01:16:17Z

@@ -1,38 +1,416 @@
 # Developer Guide


Perhaps the Developer Guide could start off with a quick introduction to what your product is about.

yuzhongng · 2022-03-31T01:25:53Z

+
+### TextUI Component
+![TextUI Component Screenshot](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/TextUI%20Component.drawio.png)
+<br>


The screenshot is not displayed in the Developer Guide webpage.

I will assume that the screenshot is a diagram that explain how the TextUI can be changed without affecting the rest of the program.

yuzhongng · 2022-03-31T01:40:14Z

+
+### Parser Component
+![Parser Component Screenshot](https://raw.githubusercontent.com/AY2122s2-cs2113t-t10-1/tp/master/docs/images/developerguide/ParserComponent.drawio.png)
+<br>


Perhaps the PaserUtils could be changed to a static class

yuzhongng · 2022-03-31T01:50:15Z

+The general workflow of the `Parser` component is as follows:
+1. When required to parse for a command, the running `SplitLah` object passes a `String` object containing
+   the user input to `Parser` class.
+2. `Parser` class instantiates a new `XYZCommandParser` object corresponding to the user input 


Is the XYZCommandParser referring to the chunk of code below?

Update user guide and developer guide

Update Developer Guide

Update printing format

Update ActivityTesting section

Update user guide formatting

Update ppp

Update PPP

Update ppp

Minimise PPP

Fix typo in PPP

Update ppp indentation

Update PPP

BradenTeo reviewed Mar 31, 2022

View reviewed changes

laiisaac reviewed Mar 31, 2022

View reviewed changes

cczhouqi reviewed Mar 31, 2022

View reviewed changes

ibrahimisramos reviewed Mar 31, 2022

View reviewed changes

yuzhongng reviewed Mar 31, 2022

View reviewed changes

froststein and others added 25 commits April 10, 2022 21:01

Add test section for session delete command

4dd8583

Add test section for session edit command

d6b0d74

Merge pull request #484 from warrencxw/branch-cleanup

76a2b5b

Update user guide and developer guide

Update to sort in alphabetical order

2756285

Update test cases to follow overall standardisation

d8526ab

Update expected.txt to include dollar sign

6ec2dcc

Amend grammar

bd7bd24

Add for test cases for session edit command in DG

8b47b89

Add test section for group create command

59dc1b1

Add test section for group delete command

62773c1

Add test section for group edit command

affd680

Add test section for storage

00b8983

Add trailing white spaces

7e61090

Amend sentence structure

81c4b1f

Amend more sentence structures

892a06f

Amend spelling mistake

fb94991

Amend phrasing

ee611b1

Merge branch 'master' into docs-developerguide

892486a

Merge pull request #487 from froststein/docs-developerguide

07a9f31

Update Developer Guide

Merge pull request #486 from ivanaitzliddat/branch-activity

3186f5d

Update printing format

Update user stories to remove capitalisation of user

bd56f3a

Merge branch 'master' into branch-developerGuide

c4e457b

Update test cases to follow overall standardisation

9eb3ee2

Fix error in test scenario 3 of session create

fe652ae

Move isSamePersonList method to PersonList object class

d3c8974

eigne and others added 30 commits April 11, 2022 22:52

Merge branch 'master' into DG

990fbee

Merge pull request #505 from matheril/DG

18a4e7d

Update ActivityTesting section

Merge pull request #501 from warrencxw/branch-cleanup

f734a70

Update user guide formatting

Merge branch 'master' into branch-ppp

437545f

Remove <hr>

41a3261

Update ppp to fit page limit

05cd543

Remove unnecessary section

fc892ee

Update PPP to exclude additional sections

da23b70

Add title

cbd95ee

Minimise PPP

369571f

Clean up PPP

08e8faf

Clean up PPP

548012f

Update ppp

faf7a3c

Update ppp

c6dcafe

Further minimisation of PPP

780a877

Merge pull request #507 from ivanaitzliddat/branch-ppp

5a87788

Update ppp

Merge pull request #508 from lelerer/PPP

7c46a09

Update PPP

Merge pull request #509 from froststein/about-us

d8a993f

Update PPP

Merge pull request #510 from matheril/ppp

eea6489

Update ppp

Update indentation of header

3375ffb

Fix typo

eafd52e

Merge branch 'master' into branch-ppp

2d6842d

Merge pull request #511 from warrencxw/about-us-Warren

d7de16b

Minimise PPP

Merge pull request #513 from matheril/ppp

f847f60

Fix typo in PPP

Merge pull request #512 from ivanaitzliddat/branch-ppp

d74414a

Update ppp indentation

Remove one line

871d25c

Remove name

008a782

Fix ppp

db09f09

Fix ppp

7427228

Merge pull request #514 from matheril/ppp

ef369f8

Update PPP

Uh oh!

Conversation

froststein commented Mar 3, 2022

Uh oh!

BradenTeo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laiisaac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cczhouqi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibrahimisramos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants