[CS2113-T10-3] MangaTantou #9

xenthm · 2024-09-19T02:59:59Z

MangaTantou serves to manage the tasks under which editors in a manga company have to handle. This includes having to chase authors for deadlines, track sales revenue, manage authors’ salaries etc.

thoriritu · 2024-10-26T17:45:55Z

src/main/java/parser/Parser.java

+import static constants.Regex.PRICE_OPTION_REGEX;
+import static constants.Regex.QUANTITY_OPTION_REGEX;
+import static constants.Regex.SPACE_REGEX;
+


Consider using enums instead of static constants like CATALOG_COMMAND to improve maintainability.

thoriritu · 2024-10-26T17:47:35Z

src/main/java/parser/Parser.java

+     *                  which should specify an Add operation for either Manga or Author
+     * @return a Command object representing the Add operation for Manga or Author
+     * @throws TantouException if the user input is invalid for both Manga and Author Add commands
+     */


consider extracting code (within each block) to individual methods if possible

thoriritu · 2024-10-26T17:51:12Z

src/main/java/parser/Parser.java

+            return new AddSalesCommand(salesArguments);
+        }
+        throw new TantouException("Invalid sales command provided!"
+                + " You need to provide the author, manga, quantity sold, and unit price.");


Consider splitting the class as it has many functionality and can be separated into 2 different ones.

thoriritu · 2024-10-26T17:59:09Z

src/main/java/storage/MangaDeserializer.java

+            throw new JsonParseException("corrupt Manga object");
+        }
+        JsonObject mangaJsonObject = json.getAsJsonObject();
+


Instead of generic messages like "corrupt manga name", provide more informative ones that specify which field was invalid.

Thanks for the feedback! The part you are referencing checks if the Manga entry in the data file is a JsonObject (whichs means it shouldn't be formatted as an array or anything else. I provided more informative messages if the mangaName or deadline contains issues in the following lines of code.

thoriritu · 2024-10-26T18:10:45Z

src/main/java/commands/DeleteMangaCommand.java

+                saveFile(authorList);
+                return;
+            }
+            assert !existingAuthor.hasManga(deletingManga): "No manga found";


The assertion check for finding the existence of authorName after already validating it before could be removed since they are unnecessary

chewycj

Overall, the diagrams are really clear and detailed!! 💯 Just some minor notations to change 😄 We found this from a past year forum that might be useful for formatting the diagrams: nus-cs2113-AY2324S2/forum#35

chewycj · 2024-10-30T02:35:32Z

docs/uml/images/mangasales_class.png

The class diagram is really detailed and well-drawn! 🚀 However, there were some non-standard notation for the diagram mangasales_class.png to note, according to prof, I think can refer to how the team did the command.png for the standard format too! 👍

Class name:

Perhaps for normal classes, I think we don't need to coloured green C!
such as from the addressbook example:

Visibility:

The symbols for the visibility could be changed to '+' and '-' or '#' accordingly too!

Yes! This will be fixed once we include our standardised config file to the puml files.

chewycj · 2024-10-30T02:47:56Z

docs/uml/images/AddAuthorSequence.png

All of the team's sequence diagrams are well drawn and neat! 💯 One thing to note that prof has pointed out is that the boxes of the entities are rounded, as it should be a sharp corner instead!

For example, these are rounded:

Noted on this, thanks for catching it!

chewycj · 2024-10-30T02:50:42Z

docs/DeveloperGuide.md

+> **_NOTE:_** There is circular reference (bidirectional navigability) between `Author` and `Manga` through `MangaList`. 
+
+### Commands 
+![Command Inheritance](/docs/uml/puml/Command/Command.png)


Perhaps the Command.png can be placed in the images folder! As currently it could not load in the DG 😿

Ah, careless mistake! Thanks for helping us point it out early!

chewycj · 2024-10-30T02:56:42Z

docs/uml/puml/AuthorListClass/AuthorListClass.png

Perhaps the UML note can be connected by a dotted line instead!

Thank you, will look into that!

jemehgoh

Good job on your guide! The explanations are mostly relevant and the diagrams are mostly appropriate. I just have several nits, as mentioned in my other comments.

jemehgoh · 2024-10-30T05:16:41Z

docs/DeveloperGuide.md

+inheriting from the abstract `Command` class. Ensure that each new command class includes 
+an implementation of the `execute` method and appropriately interacts with the `Ui` class 


I think you should not repeat yourself here regarding implementing the "execute" method?

Thanks for the comment, I agree that the content is similar in nature regarding the execute method but in the Guidelines, I had phrased it and targetted my language to be instructional towards the developers while in Structure, it is more of a generic description. Nonetheless, I hear you and will look into making the text more targetted with reduced repetition. Thanks again!

jemehgoh · 2024-10-30T05:22:06Z

docs/DeveloperGuide.md

+For the AddSalesCommand to be successful, the manga that the sales data is associated with must exist. If the `sales`
+command is successful, the `Sales` data is then saved via Storage.
+
+![mangasales_class.png](uml/images/mangasales_class.png)


I think this class diagram is not relevant to this section and shouldn't be placed here?

jemehgoh · 2024-10-30T05:30:12Z

docs/uml/puml/AuthorListClass/AuthorListClass.puml

+class ArrayList<T>
+ArrayList <|-- AuthorList
+note on link : ArrayList<Author>
+ArrayList <|-- MangaList
+note on link : ArrayList<Manga>


In this diagram, MangaList and AuthorList are indicated as inheriting from ArrayList, but with notes stating "ArrayList" and "ArrayList". I think that, unless these two classes inherit from ArrayList (in which I think you should remove the notes), I believe that there is no inheritance to indicate?

Sorry, I'm not sure I understand you. They both inherit from ArrayList, but AuthorList extends ArrayList<Author>, while MangaList extends ArrayList<Manga>. I still do believe the notes are necessary for show that distinction.

Will be keeping this representation based on:

Your notation seems OK (* good!). Not sure what the comment means specifically.

From UML specification:

*we don't cover this in CS2113; so you went beyond to learn the notation for generics inheritance.
specifying the type in the UML note seems OK to me as a way to explain to the reader what it means.

Originally posted by Prof in nus-cs2113-AY2425S1/forum#41 (comment)

jemehgoh · 2024-10-30T05:40:14Z

docs/uml/puml/ViewMangaSequence/ViewMangaSequence.puml

+        cmd -> ui : print list as table with the required columns
+            ref over ui, col : Print all required list and format rows
+        return
+return done printing


I think you should remove the "done printing" label, as the dotted arrow is sufficient to indicate that control has gone back to Tantou?

jemehgoh · 2024-10-30T05:46:09Z

docs/uml/puml/ViewMangaSequence/refGetColumnsToPrintSequence.puml

+group sd [Add required columns to list]
+    ml -> row ** : add new PrintColumn
+    row ++
+    return added to columns


I think you don't have to mention "added to columns" every time you return control, since you already mention "add new [column]" for each corresponding operation?

You're right. Perhaps I can make it clearer by appending "to columns" to each top arrow.

lowjunchen · 2024-10-31T13:01:56Z

docs/DeveloperGuide.md

+
+#### Storage Structure
+![StorageClass.png](uml/puml/StorageClass/StorageClass.png)
+The above UML class diagram outlines the structure of the classes related to saving data. 


This line is not well formattted. I think it is meant to start on the next line

lowjunchen · 2024-10-31T13:06:33Z

docs/DeveloperGuide.md

+#### Interaction
+The following diagram illustrates the interactions that take place when the
+user provides `"catalog -a Kubo Tite"` as an input.
+![add author sequence diagram](/docs/uml/images/AddAuthorSequence.png)


This is a broken link which is not visible.

lowjunchen · 2024-10-31T13:10:40Z

docs/DeveloperGuide.md

+For the AddSalesCommand to be successful, the manga that the sales data is associated with must exist. If the `sales`
+command is successful, the `Sales` data is then saved via Storage.
+
+![mangasales_class.png](uml/images/mangasales_class.png)


May I clarify what is the meaning of the circle and square icons? If I remember correctly it should be + or - or omitted

It is the default way for PlantUML to display the access modifier. But you are correct, we should be following the textbook for this course.

lowjunchen · 2024-10-31T13:22:21Z

docs/DeveloperGuide.md

+
+The following sequence diagram illustrates the interactions that occur when the parser creates a new `AddSalesCommand`.
+
+![addsalesdata.png](uml/images/addsalesdata.png)


Is the sequence diagram format correct? Should it not have the instance at the bottom?

You are right, it should not be there. Thank you.

Updated schedule to work with the new parser

DG - Schedule

…luded

Branch stress test

- Wrap long lines - Fill in Product Scope section - Fill in User Stories section - Fill in Non-Functional Requirements section - Add instructional placeholder text for Manual Testing section

- Fix "RegEx" typos - Update table of contents

- Delete template PPP - Add Ian's file

Cut lines and reformat.

Branch finalise dg

Donovan's PPP

Give instructions on how to run the jar file.

- Adding sales data with command did not show the numbers that were just added as a message. This was fixed in AddSalesCommand.java - In SaleDeserializer.java, perform checks on the saleJsonObject before proceeding

Fix Sarah's PPP

Final fixes

Remove redundant spaces in code blocks in UG

The previous one did not work.

Try another horizontal line in DG

Fixes to images in DG

GitHub should look at the docs directory to find the updated README.md

Delete README.md

thoriritu reviewed Oct 26, 2024

View reviewed changes

chewycj reviewed Oct 30, 2024

View reviewed changes

jemehgoh reviewed Oct 30, 2024

View reviewed changes

This was referenced Oct 31, 2024

Address DG comments made by TA and peers AY2425S1-CS2113-T10-3/tp#94

Closed

How do we represent inheritance from a generic class? nus-cs2113-AY2425S1/forum#41

Open

lowjunchen reviewed Oct 31, 2024

View reviewed changes

averageandyyy and others added 24 commits November 4, 2024 12:36

Update Main to use Tantou

1dc75c8

Merge branch 'master' into schedule_updated_parser_fixed

afb4526

Fixed import issues caused by branch merge

afa244f

Removed unused import

8909a6a

Merge pull request #99 from iaso1774/schedule_updated_parser_fixed

a7b4168

Updated schedule to work with the new parser

Included config file based on suggestion by @xenthm

05711a7

Included config file based on suggestion by @xenthm

4eb29c7

Merge branch 'master' into DG

cff9b55

Updated schedule.png

9ab771b

Merge remote-tracking branch 'origin/DG' into DG

1f0ac65

Merge pull request #101 from iaso1774/DG

e1c5c83

DG - Schedule

Update Deadline tests to check for the right exception being thrown

1c0cf9b

Remove unused import

c3a28a7

Update Tantou references to MangaTantou

dee05e7

Update parsing regex to stop at all valid flags, flag of interest inc…

c1fb4f2

…luded

Rename MangaTantou to Tantou

e52e68b

Merge pull request #114 from averageandyyy/branch-StressTest

1d4d20a

Branch stress test

Update UML diagrams to match new Sale class

05627d1

Update DG

4f4cd1a

- Wrap long lines - Fill in Product Scope section - Fill in User Stories section - Fill in Non-Functional Requirements section - Add instructional placeholder text for Manual Testing section

Add table of contents to DG

bb42ba5

Add table of contents to DG

446da87

Update DG with new data file format

5b53ee4

Add field extractor regex explanation

168ae33

Update capitalization of manual testing section

5893eca

averageandyyy and others added 30 commits November 11, 2024 23:32

Merge github.com:AY2425S1-CS2113-T10-3/tp into branch-FinaliseDG

e11e56b

Update images with CommandValidator

92e4957

Merge remote-tracking branch 'upstream/master' into branch-ppp

2cde663

Update DG

b946eef

- Fix "RegEx" typos - Update table of contents

Update AboutUs.md

4564d37

- Delete template PPP - Add Ian's file

Fix minor typos in DG

e716a45

Update link to UserGuide.md in DG

9c3c783

Update images

01e7269

Update names in AboutUs.md

ced27d8

Update PPP

04e9760

Cut lines and reformat.

Merge pull request #184 from averageandyyy/branch-FinaliseDG

8ecc2ab

Branch finalise dg

Revert to list in PPP

b8f52df

Merge pull request #119 from xenthm/branch-ppp

8d5e8be

Donovan's PPP

Fix link in AboutUs.md

4ea9d9f

Merge remote-tracking branch 'upstream/master'

c5e68d4

Insert horizontal divider between consecutive diagrams

e234943

Remove point from note

01cd608

Update Quick Start in UG

ae814c1

Give instructions on how to run the jar file.

Fix sales parsing and data reading

5dba0b5

- Adding sales data with command did not show the numbers that were just added as a message. This was fixed in AddSalesCommand.java - In SaleDeserializer.java, perform checks on the saleJsonObject before proceeding

Merge pull request #187 from sarahchow03/branch-fixPPP

376cc2d

Fix Sarah's PPP

Merge pull request #185 from xenthm/branch-ppp

54e729e

Final fixes

Merge remote-tracking branch 'upstream/master'

d821e2b

Remove redundant spaces in code blocks in UG

e45ab71

Merge pull request #188 from xenthm/branch-ppp

69decef

Remove redundant spaces in code blocks in UG

Try another horizontal line in DG

09c37dd

The previous one did not work.

Merge pull request #189 from xenthm/branch-ppp

07088ac

Try another horizontal line in DG

Fix images with cut-off activation bars

0d09e85

Merge pull request #190 from sarahchow03/branch-imgFix

c9a45d6

Fixes to images in DG

Delete README.md

ba76d5a

GitHub should look at the docs directory to find the updated README.md

Merge pull request #191 from AY2425S1-CS2113-T10-3/xenthm-patch-1

4fd0901

Delete README.md

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CS2113-T10-3] MangaTantou #9

[CS2113-T10-3] MangaTantou #9

xenthm commented Sep 19, 2024

thoriritu Oct 26, 2024

thoriritu Oct 26, 2024

thoriritu Oct 26, 2024

thoriritu Oct 26, 2024

xenthm Oct 31, 2024

thoriritu Oct 26, 2024

chewycj left a comment •

edited

Loading

chewycj Oct 30, 2024

xenthm Oct 31, 2024

chewycj Oct 30, 2024

averageandyyy Oct 31, 2024

chewycj Oct 30, 2024

averageandyyy Oct 31, 2024

chewycj Oct 30, 2024

xenthm Oct 31, 2024

jemehgoh left a comment

jemehgoh Oct 30, 2024

averageandyyy Oct 31, 2024

jemehgoh Oct 30, 2024

jemehgoh Oct 30, 2024

xenthm Oct 31, 2024

xenthm Nov 2, 2024

jemehgoh Oct 30, 2024 •

edited

Loading

jemehgoh Oct 30, 2024 •

edited

Loading

xenthm Oct 31, 2024

lowjunchen Oct 31, 2024

lowjunchen Oct 31, 2024

lowjunchen Oct 31, 2024

xenthm Oct 31, 2024

lowjunchen Oct 31, 2024

xenthm Oct 31, 2024

		inheriting from the abstract `Command` class. Ensure that each new command class includes
		an implementation of the `execute` method and appropriately interacts with the `Ui` class


		The following sequence diagram illustrates the interactions that occur when the parser creates a new `AddSalesCommand`.

		![addsalesdata.png](uml/images/addsalesdata.png)

[CS2113-T10-3] MangaTantou #9

Are you sure you want to change the base?

[CS2113-T10-3] MangaTantou #9

Conversation

xenthm commented Sep 19, 2024

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

chewycj 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

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

jemehgoh 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

jemehgoh Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

jemehgoh Oct 30, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chewycj left a comment •

edited

Loading

jemehgoh Oct 30, 2024 •

edited

Loading

jemehgoh Oct 30, 2024 •

edited

Loading