-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CS2113-T15-3] Module Manager #27
base: master
Are you sure you want to change the base?
[CS2113-T15-3] Module Manager #27
Conversation
Fix issues to pass CL test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some comments and suggestions on the developer guide. Good job on the overall format of the developer guide, which seems good and neat. The explanations are also concise.
2) Added using `add id/ID s/SEMESTER mc/MODULE_CREDIT` command | ||
|
||
Step 2: | ||
User executes `CAP` command to view his own CAP. The `CAP` commands is parsed through `Parser`, which would then return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the DG looks well-organised and informative. Some areas can be clearer and the sequence diagrams may need a bit of tweaking.
docs/DeveloperGuide.md
Outdated
The user launches the application for the first time. The `SemesterList` will be initialized with the none | ||
`SemModulesList` in it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to say "empty SemModulesList"?
docs/DeveloperGuide.md
Outdated
`AddToSemCommand#execute()` then calls self method `AddToSemCommand#addModule()`.`AddToSemCommand#addModule()` | ||
then calls `AddToSemCommand#checkModuleExist(semesterList)` to check whether the selected | ||
module is already in the selected module list (i.e:`semesterList`, which is a `PriorityQueue<SemModulesList>`). | ||
If the module is not in the list, `AddToSemCommand#addModule()` will check whether there is a semester list | ||
(i.e:`semesterModulesList`, which is a `ArrayList<SelectedModule>`) whose name is the module's semester name. | ||
If the semester list exist, the module will be added to the list. | ||
If not, `AddToSemCommand#addModule()` will create a new semester list and then add this module to the new list. and the | ||
the new semester list will be added to `semesterList` as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this section quite confusing to read as there are a few variables and classes with similar names (semesterList, SemModulesList, semesterModulesList, SemesterList), and the multiple 'if' condition checks. Perhaps it would help to briefly explain the connection between semesterList and semesterModulesList, at the very beginning, before delving into the logic? I.e. "semesterList represents XXX and stores instances of SemModuleList. semesterModulesList represents XXX and stores XXX"
docs/DeveloperGuide.md
Outdated
user | ||
|
||
The following sequence diagram shows how the `Add to Semester` operation works: | ||
![Sequence Diagram of Add to Semester](https://github.com/RenzoTsai/tp/blob/Update_DG/docs/UML%20img%20folder/Sequence%20Diagram%20of%20Add%20to%20Semester.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with a pattern of `#.00`. `Ui.showcap(cap)` is called to display the user's cap using the formatted `String`. | ||
|
||
The following diagram shows how the Calculate CAP operation works: | ||
![Calculate CAP feature](https://github.com/bennychanya/tp/blob/master/CalculateCap.png?raw=true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with what Isabella said about the construction of CalculateCapCommand. Otherwise it looks good to me. Perhaps you may want to specify the return value (grade) of the getGrade() method to be consistent, as you did so for totalCap?
docs/DeveloperGuide.md
Outdated
user | ||
|
||
The following sequence diagram shows how the `Add to Semester` operation works: | ||
![Sequence Diagram of Add to Semester](https://raw.githubusercontent.com/RenzoTsai/tp/master/docs/UML%20img%20folder/Sequence%20Diagram%20of%20Add%20to%20Semester.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be the case that steps also refer tot the Sequence Diagram while explaining the Add To Semester
feature?
|
||
Step 2: | ||
User executes `CAP` command to view his own CAP. The `CAP` commands is parsed through `Parser`, which would then return | ||
`CalculateCapCommand()`. `CalculateCapCommand.execute()` is then called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if it is fully clear weather object representing CalculateCapCommand class or a Person Object Calculates the total cap. More specifically, the setTotalCap() method and getTotalCap() methods are pointed in the same direction, so I wasn't a 100% sure from the diagram where the CAP calculation was happening.
docs/DeveloperGuide.md
Outdated
The `Model` component | ||
* Stores a `Person` object that represents the user's details. (e.g. `totalCap`, `totalModuleCreditCompleted`) | ||
* Stores the `ModuleList` and `SemesterList` Data | ||
* Does not depend on any of the other three components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if Model component is not related to other three components, then how would a Person object, which is stored in Model Component, have their CAP calculated? Maybe "does not depend" is not the correct phrase here?
docs/DeveloperGuide.md
Outdated
|
||
### Implementation | ||
|
||
#### `Add to Semester` feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diagram looks very neat and helps the developer understand the basic logic of add to semester future. So perhaps, is the explanation repetitive here?
Fix some bugs
…r credits and semester values
Updated Developer Guide
Allow to show prerequisites which are not in the available modules list and fix a bug when add same modules to AML
Updated Developer Guide and Architecture.png
Updated DG with design consideration for Mark As Done
Added Viewing in DG and design considerations
Update Benny's PPP
Add design considerations
Update add‘s design consideration in DG
updated deetomok.md
Added table for Design consideration
updated deetomok.md
update AboutUs.md
update deetomok.md
update deetomok.md
update UG
@bennychanya @chengTzeNing @DeetoMok