[CS2113T-T12-1] mTracker by theodorekwok · Pull Request #32 · nus-cs2113-AY2122S1/tp

theodorekwok · 2021-09-30T04:22:39Z

mTracker is a command-line based trading journal interface that allows investors and traders to store and view important trading related information on their shortlisted financial instruments for reference and decision-making. It summarises key details into an easy-to-read format and helps to track profits/losses, thus providing convenient lookups for busy individuals.

Kair0s3

Overall, the developer guide provides a wide overview as well as some of the implementation/design rationales behind the components in the application. There was a good use of colours as well to "isolate" the different components for reader to follow.

However, some of the things that I felt could be improved in my opinion would be consistency and maybe reducing/decomposing some of the complex diagrams (which may help in readability).

But again, overall, good job and have a nice day!

Kair0s3 · 2021-10-28T02:09:02Z

+## Design
+
+> Tip: The diagrams in this guide were designed using PlantUML.
+> Their original .puml files can be found in the diagrams folder here.


I think that this might be due to time constraints but, maybe the "here" could be hyperlinked?

Kair0s3 · 2021-10-28T02:12:06Z

+
+Major components of the app:
+* `MTracker` contains the `main` method responsible for launching and 
+running the app. It first initializes the other components in the correct sequence


The use of "correct sequence" seems to be confusing since there isn't any part telling the read the "correct sequence" to initialize, maybe a general idea of initialization would be sufficient?

Kair0s3 · 2021-10-28T02:18:05Z

+instructions for user input, and other display texts. The class contains both
+  strings of commonly used display texts like the console input prompter, and 
+  methods that print these strings out, thus ensuring satisfactory user interface and 
+  communication with user.


Maybe instead show the user communicating with the UI on the architecture diagram instead? (However I do understand that there might have been some rationale being doing the explanation in terms of word.)

eg.

Sorry for the bad drawing

This is diagram has its good points as it has colours to signify the different components which the system and the colours matches those in the other diagrams.

The only exception of the mismatch of colours would more likely be the ones in the sequence diagrams, though I understand that this could be due to readability reason of the sequence diagram. Perhaps, a different colour scheme could be used for Ui. Or have a note on the sequence diagram - similar to like a legend and explain why the use of another shade colour for the component.

Kair0s3 · 2021-10-28T02:19:32Z

+* `filemanager` is responsible for saving the session's instruments data to local file, updating
+them during runtime, and restoring data from previous session when the program is relaunched.
+
+The subsequent sections will elaborate on the more technical design and implementation details of


Really good to have this here, especially to "guide" the reader on what the subsequent parts will be focusing on, in relation to this architecture.

Kair0s3 · 2021-10-28T02:30:21Z

+The figure below represents the class diagram of how all the parser classes interact with classes outside the `console`
+package:
+
+<img src="images/ConsoleDiagram.png" width="550"/>


Maybe an additional note or a line of clarification after the digram would be nice to explain why the class diagrams themselves do not contain any methods/variables.

Should the lines between AddXYZParser and AddInstrumentParser, and AddInstrumentParser and InputParser be solid instead of dashed as it is an inheritance association and not an interface implementation association?

Kair0s3 · 2021-10-28T02:31:13Z

+
+### Parser component
+
+The main parent class in `console` package is the `InputParser` class which is defined in `InputParser.java`.


Great to have this here to tell reader what this class/component does and what the following diagram is for.

Kair0s3 · 2021-10-28T02:32:18Z

+Two alternatives to get the instrument information from the user were considered. The first alternative was to
+get the user to add in all the information in a single line with separators
+(for example: `stock TSLA; 909.68; negative; To buy`). This was not implemented as it is likely
+for the user to enter the parameters in the wrong order. This becomes especially problematic if there are multiple


This is good since it tells the reader the rationale being why this previous complex example was not taken into implementation.

Kair0s3 · 2021-10-28T02:45:08Z

+* The command classes are dependent on the `TextUi` class. This allows the command class to display its execution results to the user.
+
+
+The figure below represents the sequence diagram when the user executes a done command:


Maybe could give a more specific use case description, in terms of what this test case does using index? From a reader's point of view, the 1 seems be taken in as an index as of the diagram, but not much seems to be explained about it in terms of the use case, like does this 1 mean just in general any of the instruments that is first in this instrument list, maybe this could be specified here?

Maybe this half of the Command component can be shifted to after the instrument feature? That way, readers would have a rough idea of the implementation behind the instruments which can flow well into the use case of this done Command (Don't mind me too much! Just a suggestion)

Kair0s3 · 2021-10-28T03:05:11Z

+
+The figure below represents the sequence diagram when the user wants to add a stock:
+
+<img src="images/AddStockSequenceDiagram.png" width="1040"/>


This might be nit-picky, but maybe the the small "hole" between :AddStockParser and the activation bar can be removed? Similarly with the :AddStockCommand

Good use of notes to explain some of the "hidden" logic eg. the prompting of user for more input details.

justinfidelis · 2021-10-28T05:22:33Z

+The figure below represents the class diagram of how all the parser classes interact with classes outside the `console`
+package:
+
+<img src="images/ConsoleDiagram.png" width="550"/>


Should the lines between AddXYZParser and AddInstrumentParser, and AddInstrumentParser and InputParser be solid instead of dashed as it is an inheritance association and not an interface implementation association?

justinfidelis · 2021-10-28T05:31:08Z

+
+<img src="images/ConsoleDiagram.png" width="550"/>
+
+How the `InputParser` class works:


Maybe it will be helpful to include a sequence diagram to show how the different classes interact with one another during the processing of a command.

justinfidelis · 2021-10-28T05:38:24Z

+in `InstrumentManager.java` and `Instrument.java` respectively. This figure below represents the class diagram of 
+how the different class work together:
+
+<img src="images/ModelDiagram.png" width="550"/>


It may be clearer to include the main class in the diagram to show where the association arrow originates from.

Similar to the console class diagram, should the lines between Instrument and the subinstrument classes be solid instead of dashed.

justinfidelis · 2021-10-28T05:47:24Z

+
+The Command component contains all the commands classes, where its respective class is instantiated when a valid command is entered by the user. 
+
+Commands include:


Maybe it will be clearer to display the Commands in a table with a short description of what they do, either here or in an appendix.

Slight typo in AddCryptoCommand

justinfidelis · 2021-10-28T05:54:56Z

+
+The figure below represents the sequence diagram when the user executes a done command:
+
+<img src="images/DoneCryptoSequenceDiagram.png" width="1040"/>


Maybe it will be clearer if the sequence diagram leaves out less important method calls. E.g., getIndex(), getTypeIcon(), getStatusIcon(). As per the instructions here

Agreed! If you would like to go into more details on the parsing process, perhaps consider using a ref frame instead :)

nguyin · 2021-10-28T08:49:02Z

+
+<img src="images/DoneCryptoSequenceDiagram.png" width="1040"/>
+
+### FileManager Component


Maybe it will be better to leave out some of the notes as they seem quite redundant. The methods are already well named and seem descriptive enough so there is no need to have the notes. Also, I think can remove the "return" for the return arrow.

tlyi · 2021-10-29T06:36:22Z

+3) AddStockCommand
+4) ExitCommand
+5) InvalidCommand
+6) ListCommand


Is DoneCommand supposed to be listed here as well? It was mentioned in the sequence diagram below but not here, which was slightly confusing.

tlyi · 2021-10-29T06:38:19Z

+
+The figure below represents the sequence diagram when the user executes a done command:
+
+<img src="images/DoneCryptoSequenceDiagram.png" width="1040"/>


Agreed! If you would like to go into more details on the parsing process, perhaps consider using a ref frame instead :)

tlyi · 2021-10-29T06:50:28Z

+`AddInstrumentParser` support the parsing of different instruments and their parameters.
+This implementation provides greater extensibility to the add functionality to support more instrument types.
+
+Two alternatives to get the instrument information from the user were considered. The first alternative was to


While I think it is great that you explained the rationale for your specific choice of implementation, I feel that this segment could have been moved to the "Add instrument feature" segment instead so that this segment is just for showing the overall architecture of the component :)

tlyi · 2021-10-29T07:05:41Z

+`AddInstrumentParser#filterByInstrumentType()` which will then guide the user through the process of adding a new
+instrument. 
+
+The figure below represents the sequence diagram when the user wants to add a stock:


Perhaps the lifeline for AddStockParser should end after it returns the parameters?

rafaelperes

Hi T12-1. I've left some comments about your TP code. Hope it helps.

rafaelperes · 2021-10-29T07:30:52Z

+
+import java.io.ByteArrayInputStream;
+import java.time.LocalDate;
+


@kum-wh perhaps remove this extra space?

rafaelperes · 2021-10-29T07:35:21Z

+import java.util.HashSet;
+import java.util.Locale;
+
+public class EditInstrumentParser extends InputParser {


@kum-wh @KVignesh122 @KVignesh122 @williamlamjy Overall, your code is neat, congrats! But maybe you could add javadoc comments for more complicated methods that may require explanation... I saw some long chunks of code, but no comments..

# Conflicts: # docs/DeveloperGuide.md

…into branch-DG-UserStories * 'master' of https://github.com/AY2122S1-CS2113T-T12-1/tp: (164 commits) PPP Fix checkstyle errors Fix according to pr review edits edits edited to pr and resize Remove space fix i/o redirection test Fix abort in main issue Make instrument decoders attribute protected not public edited accroding to pr Improve code quality Make edit status error messages more specific and remove polling for edit past returns added ppp not finished Fix import errors and ide warnings sizing edits to diags Changes based on PR edited encoding diag Fix merge conflict ...

added java docs for filemanager

Add dg extra sections

Check the files and author tags

UG

…into branch-DG-changes * 'master' of https://github.com/AY2122S1-CS2113T-T12-1/tp: Fix according to PR Fix spacing message. Fix jar naming. Changes according to PR Some changes Fixing links Changes according to PR More of abort section Adding the abort section

diag changes

…l-change Final change

Fix DG

…atting Fix formatting issue

Fix author tags

Kair0s3 reviewed Oct 28, 2021

View reviewed changes

theodorekwok changed the title ~~[CS2113-T12-1] mTracker~~ [CS2113T-T12-1] mTracker Oct 28, 2021

justinfidelis reviewed Oct 28, 2021

View reviewed changes

nguyin reviewed Oct 28, 2021

View reviewed changes

tlyi reviewed Oct 29, 2021

View reviewed changes

rafaelperes reviewed Oct 29, 2021

View reviewed changes

wingho2 and others added 24 commits November 5, 2021 19:43

Merge branch 'master' into DG

1990bed

# Conflicts: # docs/DeveloperGuide.md

Fix merged conflict

e142649

Add TextUi seq diagrams and explanation

c46b45b

Shift Ui section to below Command Component

215d467

Enlarge diagrams

2ff9930

Decrease diagrams size

2f78f1a

Decrease diagrams size

ce8f39a

Add javadocs to loghelper

1d2e8d8

added java docs for filemanager

40c7a7a

cehcsktyle

802f6fb

Rectify PR comments

cb402f1

Add dg extra sections

edbeb5d

Remove greet msg

c72cacf

Make manual testing section bold

c0bd685

cahnges according to pr

c245593

Make launch and start up bold

83aedbf

Merge pull request #233 from williamlamjy/branch-Java-Docs

1268d09

added java docs for filemanager

Merge branch 'master' into theodorekwok-dg-extra-sections

5b91724

Merge branch 'master' into theodorekwok-loghelper-javadocs

46b6f9f

Merge branch 'master' into theodorekwok-console-javadocs

a1cc630

Merge branch 'master' into theodorekwok-author-tags

6b44ab5

Merge branch 'master' into theodorekwok-create-links-to-ppp

357a725

Merge branch 'master' into theodorekwok-fix-ug-errors

2f1203b

theodorekwok and others added 30 commits November 7, 2021 23:25

Merge branch 'master' into theodorekwok-dg-extra-sections

892bf68

Merge branch 'master' into DG

682cac5

Merge pull request #234 from theodorekwok/theodorekwok-dg-extra-sections

c57819e

Add dg extra sections

Merge branch 'master' into theodorekwok-check-author-tags

680634f

Merge pull request #247 from theodorekwok/theodorekwok-check-author-tags

4c7a523

Check the files and author tags

Merge branch 'master' into DG

5ac0f15

Fix jar naming.

5ce6931

Fix spacing message.

0765f3d

diag changes

bc95ed1

Fix according to PR

8a88181

edits

aeef968

Merge pull request #246 from kum-wh/DG

d63648a

UG

edits

de0b8f0

Merge pull request #252 from williamlamjy/branch-DG-changes

534f995

diag changes

Change to edit etf parameters so that line width is shorter

55e60c6

Fix dg

35301c3

Fix remaining dg issues

61fb482

Fix tabs

36786e9

Fix dg

185ad23

Fix tabbing in dg

1d6eef8

Merge pull request #253 from AY2122S1-CS2113T-T12-1/theodorekwok-fina…

ae4e6d3

…l-change Final change

Fix DG

3bf19e6

Update console diagram

e876c83

Merge pull request #254 from theodorekwok/theodorekwok-fix-model-diagram

fa2b657

Fix DG

Fix formatting issue

b1bf104

Merge pull request #255 from theodorekwok/theodorekwok-dg-manual-form…

a07687a

…atting Fix formatting issue

Fix author tags

589ed61

Fix more author tags

2b45f8d

Merge pull request #256 from theodorekwok/theodorekwok-fix-author-tags

8a4c907

Fix author tags


		### Parser component

		The main parent class in `console` package is the `InputParser` class which is defined in `InputParser.java`.

		* The command classes are dependent on the `TextUi` class. This allows the command class to display its execution results to the user.


		The figure below represents the sequence diagram when the user executes a done command:


		The figure below represents the sequence diagram when the user wants to add a stock:

		<img src="images/AddStockSequenceDiagram.png" width="1040"/>


		<img src="images/ConsoleDiagram.png" width="550"/>

		How the `InputParser` class works:


		The Command component contains all the commands classes, where its respective class is instantiated when a valid command is entered by the user.

		Commands include:


		The figure below represents the sequence diagram when the user executes a done command:

		<img src="images/DoneCryptoSequenceDiagram.png" width="1040"/>


		<img src="images/DoneCryptoSequenceDiagram.png" width="1040"/>

		### FileManager Component


		import java.io.ByteArrayInputStream;
		import java.time.LocalDate;

Conversation

theodorekwok commented Sep 30, 2021

Uh oh!

Kair0s3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

rafaelperes 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Kair0s3 left a comment •

edited

Loading