Skip to content
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-F11-3] YetAnotherModuleOrganiserManager #20 #32

Open
wants to merge 655 commits into
base: master
Choose a base branch
from

Conversation

amitrahman1026
Copy link

An CLI that helps students to keep track of lessons, schedules and exam timetables
An CLI that can help set reminders for students
An CLI that can help students plan for their curriculum

Copy link

@P0tatoChips P0tatoChips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No UML diagram, but so far, looks neat and readable. The alternatives considered and why is it implemented is really a nice touch.


#### 3.5.3 Storage Component

![Storage Class](..\docs\images\storageClass.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Photos does not work.


## Product scope

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Target user and value proposition not specified.

Command class and are all in the command package.

[//]: # (if the table is not necessary, remove it)
| Command | Command Type | Action |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Table not formatted properly, looks weird on the DG page.

Comment on lines 192 to 200
##### Why it is implemented this way.
User may or may not know the exact module code or title. As such, the user can search for the module based on optional
parameters such as semester or level. However, the user must input at least the module code or title before additional
parameters can be added in order to refine the search.

##### Alternatives considered.
We thought of implementing the search feature in a way that the required user for multiple inputs and displaying all the
different results after each input. However, we decided against it as it would be too tedious for the user to input
multiple times and the search process will be too long.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like how you all got add the alternatives considered, help improve understanding of your point of view and why you choose to do things.

- [3.3 Parser Component](#33-parser-component)
- [3.4 Command Component](#34-command-component)
- [3.5 Utils Component](#35-utils-component)
- [3.5.1 UI Component](#351-ui-component)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to simplify the catalogue(for example, omit the 3.5.x because these are sub-subsections), in order to have better visual experience.

### 3.2 Model Component

### 3.3 Parser Component
![Parser Class](..\docs\images\parserClass.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This image fails to show on the develper guide on my laptop.

| `bye` | `ExitCommand` | Exits the program. |
| `nil` | `InvalidCommand` | Displays the invalid command message. |
| `nil` | `IncompleteModuleCommand` | Display the incomplete command message. |
| `nil` | `UnknownCommand` | Display the unknown command message. |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table looks a bit messy on my laptop


Core program flow is managed by the Duke class.

![Main Program Flow](images/mainProgramFlow.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cross in the command class seems to overlap the activation bar

Copy link

@AaaaaronC AaaaaronC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the class diagrams should use the same software to make. The format for some classes differ from the rest.

- [Appendix C: Non-Functional Requirements](#appendix-c-non-functional-requirements)
- [Appendix D: Glossary](#appendix-d-glossary)
- [Appendix E: Acknowledgements](#appendix-e-acknowledgements)
- [Third-party libraries](#third-party-libraries)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the appendices below are duplicated. Both lead to the same component, maybe can delete the bottom one?

Below is a table of command subclasses and their respective command type. The different command types extends from the
Command class and are all in the command package.

[//]: # (if the table is not necessary, remove it)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Table is not implemented properly, does not render

The <code>AddModuleCommand</code> class extends from the <code>Command</code> class and adds the user input module into
their timetable.

![AddModuleCommand Class](..\docs\images\addModuleCommandClass.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image does not render, maybe check file path?

* *glossary item* - Definition
Special thanks to the author of the following sources for inspiration and ideas that contributed to the development of
**YAMOM**
- https://stackoverflow.com/questions/25853393

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is a link, you might want to make it selectable


#### 3.5.1 UI Component

![UI Class](images/Ui.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this image, the class diagram C should not be there

Initially, data validation was being handled by the `Parser` class, however in the principles of avoiding tight coupling
and improving cohesion, it was moved back under the `AddModuleCommand` class.

#### 3.4.2 DeleteModuleCommand

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a sequence diagram to the commands that you missed to provide further info for users

The <code>AddModuleCommand</code> class extends from the <code>Command</code> class and adds the user input module into
their timetable.

![AddModuleCommand Class](..\docs\images\addModuleCommandClass.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alt condition is a little misleading. Both conditions call on AddMessage() and they are linked by an activation box is a bit confusing to understand.


Core program flow is managed by the Duke class.

![Main Program Flow](images/mainProgramFlow.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is get/update state one function or two separate functions

| `search` | `SearchModuleCommand` | Searches similar modules based on code, title, semester or level. |
| `semester` | `SelectSemesterCommand` | Selects the semester that the user want. |
| `select` | `SelectSlotCommand` | Selects the time slot for the different lesson types. |
| `view` | `ViewCommand` | Views the user timetable with user's selected modules. |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table when rendered becomes unaligned. I've attached a picture for your reference.
Screen Shot 2022-10-27 at 5 43 36 PM

Copy link

@JordanKwua JordanKwua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good effort, the errors mostly come from the configuration settings for plantUML, the elaboration of the methods are well done.


Core program flow is managed by the Duke class.

![Main Program Flow](images/mainProgramFlow.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The end of the lifeline (cross) should be clearer, should it be overlapping the activation bar?
image


### 3.4 Command Component

![Command Abstract Class](images/commandClass.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the symbols beside the variables and methods, maybe you should check the right format to show visibility?
image

| `search` | `SearchModuleCommand` | Searches similar modules based on code, title, semester or level. |
| `semester` | `SelectSemesterCommand` | Selects the semester that the user want. |
| `select` | `SelectSlotCommand` | Selects the time slot for the different lesson types. |
| `view` | `ViewCommand` | Views the user timetable with user's selected modules. |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be like this on the actual DG? Because the formatting seems to be wrong.
image


The following sequence diagram shows how the undo operation works:

![AddModuleCommandSequenceDiagram](images/AddModuleCommandSequenceDiagram.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do consider changing the configuration of your plantUML as the diagrams are off , such as this object box at the bottom of the lifeline.
image


Core program flow is managed by the Duke class.

![Main Program Flow](images/mainProgramFlow.png)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pr1

Do remember to deactivate the activation bar so that there is closure.


### 3.4 Command Component

![Command Abstract Class](images/commandClass.png)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pr1

In this diagram, the accessibility of the fields and methods are represented by different symbols than the ones we learnt in class. Maybe you can consider disabling skinparams to use the correct symbols?


### Storage feature

!["Opening saved state"](images/storageOpenPreviousState.png)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pr1

Are these activation bars supposed to be left open? Are they supposed to have return arrows?


#### 3.5.1 UI Component

![UI Class](images/Ui.png)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these the right accessibility symbols to use?

deenliong and others added 30 commits November 6, 2022 22:01
# Conflicts:
#	src/main/java/seedu/duke/parser/Parser.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.