[CS2113T-F12-1] PlanNUS by jerroldlam · Pull Request #50 · nus-cs2113-AY2021S1/tp

jerroldlam · 2020-10-01T15:11:32Z

PlanNUS helps NUS undergraduates plan their academic journey in NUS. It is optimized for CLI users as commands can be typed in faster by undergraduates.

…l-Logging Implemented Logging for LogInCommand.java

EdmundEXE · 2020-10-28T05:17:27Z

+### Details
+
+#### Global Component
+Classes used by multiple components part of the global component of PlanNUS. This includes classes such as `App`,`Command` and `LoggingTool`. The main object classes `PartialModule`, `FullModule` and `Person` are also within the global component.


Should global be global instead?

EdmundEXE · 2020-10-28T05:21:05Z

+|`ModuleLoader`| Class representing all modules offered by NUS | `allModules` |
+|`Person`| Class representing current user's information | `currentPerson`|
+|`Ui`| Class representing java's default scanner class | `in`|
+|`String` | Class representing the module code to be added | `moduleCode`|


Should String be in the diagram?

EdmundEXE · 2020-10-28T05:35:58Z

+The following diagram summarizes what happens when the user executes a `ModuleDetailsCommand`: 
+
+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/moduleDetailsCommand_activity.png" alt="Activity diagram for View Module Details Command"/>


The size of this image seems to be different from the other similar diagrams?

EdmundEXE · 2020-10-28T05:38:14Z

+
+#### Current implementation
+
+`SetSuBySemesterCommand` is executed by `CapCalculatorApp`. It provides users with a suggestion on how they can S/U their modules added in `AcademicPlannerApp` by retrieving the `userMduleList` from  the `Person` object and filter the list according to the semester provided to get a `suList`.


Should it be userModuleList?

EdmundEXE · 2020-10-28T05:39:16Z

+
+`SetSuBySemesterCommand` is executed by `CapCalculatorApp`. It provides users with a suggestion on how they can S/U their modules added in `AcademicPlannerApp` by retrieving the `userMduleList` from  the `Person` object and filter the list according to the semester provided to get a `suList`.
+
+`suList` will then be analysed to provide user with a list of suggested S/U modules to achieve a best Cap.


Should it be CAP? To follow as per what the title said.

EdmundEXE · 2020-10-28T05:45:54Z

+The following sequence diagram shows how `ModuleDetailsCommand` works.
+
+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/moduleDetailsCommand_sequence.png"/>


Should the self-call validateModuleCode() have a return arrow, like in the other sequence diagrams?

EdmundEXE

Other than a few minor changes, I felt that the DG greatly utilises diagrams to make explanations easy to understand, great job!

dmbclub · 2020-10-28T05:23:29Z

+/**
+ * Class representing main function of PlanNUS.
+ */
+public class PlanNus {


Should these lines be solid?

And I think apps should be connected to others?

dmbclub · 2020-10-28T05:31:52Z

+        try {
+            if (moduleValidator.isModTakenByUser(moduleCode)) {
+                removeUtils.removeModuleFromUserModuleList(moduleCode);
+                System.out.println(MODULE_REMOVED);


I think for printing something. According to your design, maybe can put it in the ui class?

dmbclub · 2020-10-28T05:58:21Z

+    private String moduleCode;
+
+    public RemoveModuleCommand(ModuleLoader allModules, Person currentPerson, Scanner in, String moduleCode) {
+        this.removeUtils = new RemoveUtils(currentPerson);


Maybe can draw a diagram to show how these objects are created.

dmbclub · 2020-10-28T06:04:34Z

+     */
+    @Override
+    public void execute() throws AcademicException {
+        try {


Maybe it is alt instead of opt?

Aseanseen

Some comments to take into consideration

Aseanseen · 2020-10-29T01:36:24Z

+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/editModuleCommand_initialState.png" alt="Initial state diagram for Edit Module Command"/>
+</div>


Maybe change the boxes, as the rounded boxes are against standard uml notation

Aseanseen · 2020-10-29T01:43:07Z

+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/editModuleCommand_activity.png" alt="Activity diagram for Edit Module Command"/>
+</div>


As not everyone will know how to read this diagram, perhaps add an elaborate explanation with the diagram. I think there are multiple occurences

Aseanseen · 2020-10-29T01:51:10Z

+
+* The `global` package contains classes, exceptions and objects that are required across the whole app. 
+* The `ui` package contains the class that is responsible for sharing one `scanner` class across the whole app to prevent multiple IO streams
+* The `parser` package contains the class that handles user's app selection


Maybe try to explain every part of the diagram, app parser was not explained here. Also, not sure if there is a specific meaning to the dotted line.

Aseanseen · 2020-10-29T01:53:55Z

+### Architecture

-{Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing}
+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/Architecture.png" alt="Architecture diagram of PlanNUS"/>
+</div>
+
+The ***Architecture Diagram*** given above explains the high-level design of PlanNUS. Below is a quick overview of each component.
+
+
+
+### Overview


Maybe try to see if you can merge the 2 sections, as right now, based on the formatting it looks separate when you are actually explaining the architecture diagram

snowbanana12345 · 2020-10-29T06:21:22Z

+
+#### Global, Ui, Parser, Storage, Apps
+
+* The `global` package contains classes, exceptions and objects that are required across the whole app. 


This package is required across the whole app. But on the architecture diagram, we see that Global only has an arrow to PlanNUS which seemed to imply that it communicates only with PLanNUS and is independent of other packages.

snowbanana12345 · 2020-10-29T06:37:09Z

+Given below is an example usage scenario and how add module command behaves at each step.
+
+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/addModuleCommand_initialState.png" alt="Initial state diagram for AddModuleCommand"/>


There is arrows going into the Ui person and ModuleLoader, unclear what they are doing. The AddModuleCommand recieves some information from these 3, the diagram doesn't show what the relationship is.

snowbanana12345 · 2020-10-29T06:39:16Z

+    <img src="./images/DeveloperGuide/addModuleCommand_finalState.png" alt="Final state diagram for AddModuleCommand"/>
+</div>
+
+**Step 7** : `FileHandler`, `Logger`, `PartialModule`, `ModuleValidator`, `AddUtils` and `AddModuleCommand` are terminated.


I cannot find what ModuleValidator did in the whole process.

snowbanana12345 · 2020-10-29T06:42:23Z

+
+__Step 2:__ The `execute()` method is called from the instance of `EditModuleCommand` which only throws `AcademicException` if applicable.
+
+__Step 3:__ Method `isModTakenByUser()` of the `ModuleValidator` is called to check if the `moduleCode` entered by the user exists within the `userModuleList` and `userModuleMap`.


Show this on the diagram. The diagram gives very little information on describing this sequence of steps. Maybe label the step numbers on the diagram for better clarity.

GuoAi

The DG is very nicely done! Perhaps you could also consider to add more explanations or diagrams at places which may cause confusion to new developers.

GuoAi · 2020-10-29T06:24:02Z


-## Product scope
-### Target user profile
+* Table of contents


The table of contents is well done! I like how I can navigate to different sections by clicking on the title.

GuoAi · 2020-10-29T06:29:34Z


-{Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing}
+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/Architecture.png" alt="Architecture diagram of PlanNUS"/>


Maybe you can consider explaining here why Apps is not interacting with other packages?

GuoAi · 2020-10-29T06:34:56Z

+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/Project_structure.png" alt="Architecture diagram for ideal project structure in PlanNUS"/>
+</div>


Is App Entry Point also a package? I am a bit confused about App Entry Point in this diagram showing the interaction among packages.

GuoAi · 2020-10-29T06:36:12Z

+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/Packages_Interaction.png" alt="Sequence diagram for lifecycle of PlanNUS"/>
+</div>


This sequence diagram is very clear and informative. Well done!

GuoAi · 2020-10-29T06:39:46Z

+### Details
+
+#### Global Component
+Classes used by multiple components part of the global component of PlanNUS. This includes classes such as `App`,`Command` and `LoggingTool`. The main object classes `PartialModule`, `FullModule` and `Person` are also within the global component.


The introduction of your global component sounds a bit abstract to me. It seems to contain several classes, but I cannot get information about how these classes interact with each other and what the function of each class is. Would it be better if you insert a class diagram here?

GuoAi · 2020-10-29T06:42:18Z

+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/addModuleCommand_state6.png" alt="State diagram for AddModuleCommand Step 6"/>
+</div>


The object diagrams you use to show the state after each step are very clear!

GuoAi · 2020-10-29T06:48:11Z

+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/addModuleCommand_sequence.png" alt="Sequence diagram for AddModuleCommand"/>
+</div>


I like this diagram showing the different branches given different cases of user inputs.

GuoAi · 2020-10-29T06:51:29Z

+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/editModuleCommand_activity.png" alt="Activity diagram for Edit Module Command"/>
+</div>


In this diagram for EditModuleCommand, perhaps you could explain how a user chooses whether she wants to edit semester or grade? The conditions for these two branches are not very clear to me.

ZhongNingmou

Overall your job is good but may need to add more details.

ZhongNingmou · 2020-10-30T03:22:01Z

+* `commands`: A package that handles all executable commands given by parser
+* `commons`: A package with the utilities and shared classes across the parent package
+* `exceptions`: A package to handle all exceptions thrown across the parent package


Maybe you can explain more about "App Entry Point" because it shows in your diagram but there is no explanation about this.

ZhongNingmou · 2020-10-30T03:29:51Z

+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/Details_architecture.png" alt="Details architecture diagram of PlanNUS"/>
+</div>


I'm a little bit confused about the relation between Apps and PlanNUS because in the previous diagram "Architecture" there is no connection between Apps and other components and in this "Details" diagram there is no connection neither, maybe you can explain more about this?

ZhongNingmou · 2020-10-30T03:36:15Z

+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/addModuleCommand_sequence.png" alt="Sequence diagram for AddModuleCommand"/>
+</div>


This sequence diagram is very clear and the previous diagrams explained each step clearly! Well done.

ZhongNingmou · 2020-10-30T03:51:31Z

+__Step 4:__ `in` reads the next line of input for user's choice of modifying either the semester or grade of the selected `moduleCode`.
+
+__Step 5:__ `isValidSemester()` or `isValidGrade()` is called to validate the semester or grade entered by the user.
+
+__Step 6:__ `updateModuleSemester()` or `updateModuleGrade()` is then called to conduct necessary changes to the information by accessing the module from `userModuleMap` and `userModuleList`.


Maybe you can add diagrams of these steps.

mrwsy1

Overall well done, can consider adding more line breaks in between each section :)

mrwsy1 · 2020-10-30T06:12:47Z

-{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
+Before reading this document, you are recommended to read through the [user guide](https://ay2021s1-cs2113t-f12-1.github.io/tp/UserGuide.html).

+## Table of contents 


Would it be better if u numbered the individual sections for the table of contents? And the sections themselves

mrwsy1 · 2020-10-30T06:17:41Z

+    <img src="./images/DeveloperGuide/Architecture.png" alt="Architecture diagram of PlanNUS"/>
+</div>
+
+The ***Architecture Diagram*** given above explains the high-level design of PlanNUS. Below is a quick overview of each component.


Maybe can explain why Apps is not linked to anything else

mrwsy1 · 2020-10-30T06:27:17Z

+The following options were considered when implementing commands:
+
+* Option 1 (Current Implementation): Implementing each command as a class by itself
+    * Pros: Increases modularity of code, higher overall code quality 
+    * Cons: More complicated to implement
+* Option 2: Implementing each command as a method in a class
+    * Pros: Easier to implement
+    * Cons: Class needs to be instantiated and increases coupling, reducing testability. This method also decreases SLAP.


Maybe u can put this into table format so it's easier to read and do comparison between pros and cons

mrwsy1 · 2020-10-30T06:31:15Z

+Given below is an example usage scenario and how add module command behaves at each step.
+
+<div style="text-align:center">
+    <img src="./images/DeveloperGuide/editModuleCommand_initialState.png" alt="Initial state diagram for Edit Module Command"/>


Can have a caption underneath the image to relate it to the steps, otherwise a little confusing

[UG, DG] Readability improvements

# Conflicts: # docs/UserGuide.md

Update UG after PE dry run

Update PPP

…into WIP-Jerrold

Fix bugs when shifting modules

Update PPP and AboutUs

Fix Bug in storage check

…into WIP-Jerrold

Fix handling of duplicated modules

Add further checks for adding modules

Add cautions in UG

Fix Capcalc message

[PPP] fixed broken reposense link

Updated PPP

Fix bug

Bug Fixes

yeapcl added a commit to yeapcl/tp that referenced this pull request Oct 14, 2020

Merge pull request nus-cs2113-AY2021S1#50 from manuelmanuntag96/Manue…

bd53c87

…l-Logging Implemented Logging for LogInCommand.java

EdmundEXE reviewed Oct 28, 2020

View reviewed changes

dmbclub reviewed Oct 28, 2020

View reviewed changes

Aseanseen reviewed Oct 29, 2020

View reviewed changes

snowbanana12345 reviewed Oct 29, 2020

View reviewed changes

GuoAi reviewed Oct 29, 2020

View reviewed changes

ZhongNingmou reviewed Oct 30, 2020

View reviewed changes

mrwsy1 reviewed Oct 30, 2020

View reviewed changes

jerroldlam and others added 16 commits November 3, 2020 11:55

Merge pull request #242 from vanessa-kang/WIP_van

a855dea

[UG, DG] Readability improvements

Merge branch 'master' of https://github.com/AY2021S1-CS2113T-F12-1/tp

6bc298c

# Conflicts: # docs/UserGuide.md

Update UserGuide.md

d70f214

Update UserGuide.md

1e96945

Update UserGuide.md

b372e79

Update UserGuide.md

4c90dbe

Update pictures for UG

ec567ba

Merge branch 'master' of https://github.com/JuZihao/tp

e91f80d

Update UserGuide.md

480a0a3

Update UserGuide.md

e650202

Merge pull request #243 from JuZihao/branch-V2.1

fc7e801

Update UG after PE dry run

Update juzihao.md

11413ba

Merge branch 'master' into branch-V2.1

d8b9094

Merge pull request #244 from JuZihao/master

f6c133d

Update PPP

[DG] Added manual testing instructions for acadplanner commands.

8688015

[DG] Fixed formatting issues in use case and manual testing sections.

15e3d45

jerroldlam and others added 30 commits November 9, 2020 18:37

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

3520878

…into WIP-Jerrold

Merge pull request #314 from jerroldlam/WIP-Jerrold

2776ce8

Fix bugs when shifting modules

Update PPP

470229d

Resize aboutus pictures

58f3769

Update PPP

b4e5bf1

Merge pull request #315 from jerroldlam/WIP-Jerrold

47ba12f

Update PPP and AboutUs

Fixed Bug in storage check

e16fd46

Fixed storage checks bugs

4075344

Merge pull request #316 from Khenus/Khenus

7a3f65a

Fix Bug in storage check

Fix handling of duplicated failed modules

762df17

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

ceac8ae

…into WIP-Jerrold

Fix handling of duplicated modules

32a0bc1

fix checkstyle

1e7d407

Merge pull request #317 from jerroldlam/WIP-Jerrold

20e6027

Fix handling of duplicated modules

Added further checks for adding modules

d7c16e6

Merge branch 'master' into Khenus

4ca36cf

Merge pull request #318 from Khenus/Khenus

095319a

Add further checks for adding modules

Add cautions in UG

4e4d2e9

Merge pull request #319 from jerroldlam/WIP-Jerrold

98935e1

Add cautions in UG

Fix Capcalc message

2f391ce

Fix JUnit test for Capcalc parser

c2845fd

Merge pull request #320 from jerroldlam/WIP-Jerrold

b496174

Fix Capcalc message

[PPP] update reposense link

1311305

Merge pull request #321 from vanessa-kang/WIP_van

11c23da

[PPP] fixed broken reposense link

Updated PPP

69c463b

Merge pull request #322 from Khenus/Khenus

30fb7f5

Updated PPP

Fix bug

dafb591

Merge pull request #2 from jerroldlam/WIP-Jerrold

28d0d83

Fix bug

Update AddModuleCommand.java

82ab12e

Merge pull request #324 from Khenus/Khenus

6a55d2b

Bug Fixes


		#### Current implementation

		`SetSuBySemesterCommand` is executed by `CapCalculatorApp`. It provides users with a suggestion on how they can S/U their modules added in `AcademicPlannerApp` by retrieving the `userMduleList` from the `Person` object and filter the list according to the semester provided to get a `suList`.


		`SetSuBySemesterCommand` is executed by `CapCalculatorApp`. It provides users with a suggestion on how they can S/U their modules added in `AcademicPlannerApp` by retrieving the `userMduleList` from the `Person` object and filter the list according to the semester provided to get a `suList`.

		`suList` will then be analysed to provide user with a list of suggested S/U modules to achieve a best Cap.


		#### Global, Ui, Parser, Storage, Apps

		* The `global` package contains classes, exceptions and objects that are required across the whole app.


		__Step 2:__ The `execute()` method is called from the instance of `EditModuleCommand` which only throws `AcademicException` if applicable.

		__Step 3:__ Method `isModTakenByUser()` of the `ModuleValidator` is called to check if the `moduleCode` entered by the user exists within the `userModuleList` and `userModuleMap`.

Uh oh!

Conversation

jerroldlam commented Oct 1, 2020

Uh oh!

EdmundEXE Oct 28, 2020 • 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!

EdmundEXE Oct 28, 2020 • 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!

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

dmbclub Oct 28, 2020 • 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!

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

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

ZhongNingmou 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

EdmundEXE Oct 28, 2020 •

edited

Loading

EdmundEXE Oct 28, 2020 •

edited

Loading

dmbclub Oct 28, 2020 •

edited

Loading

Aseanseen left a comment •

edited

Loading