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

[CS2113T-F08-1] ModMan #30

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

Conversation

jianningzhuang
Copy link

Mod Man, short for Module Manager, is a desktop app designed to help teaching assistants manage their module(s). It is optimized for use via a Command Line Interface (CLI). Mod Man helps to track module details as well as students’ progress and data, all in one platform.

Copy link

@brynagoh brynagoh left a comment

Choose a reason for hiding this comment

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

Overall, your team's developer guide is rather clear and easy to understand.

Comment on lines 47 to 55
1. `Ui` is class with a `Scanner` object that reads input from console.
1. `Modman` has `Ui` object as an attribute which is instantiated
when we enter the program.
1. `Ui` object reads input from screen and returns it which will then be passed to the Parser so that it can
parse the command.
1. Depending on the parsed command, the corresponding
`execute()` function of the `Command` abstract class
runs. It accepts `Ui` object as a parameter and calls the relevant `Ui` methods
that prints the messages.
Copy link

Choose a reason for hiding this comment

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

Maybe include a sequence diagram to illustrate this?

Comment on lines 72 to 74
The `Data`,
* stores an `ArrayList` of `Module` objects that represents the modules.
* Each `Module` object contains `ArrayLists` of `Assignment`, `Lesson`, and `Student` objects.
Copy link

Choose a reason for hiding this comment

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

Maybe include a line to explain moduleCode, as shown in the diagram?

Comment on lines +83 to +84
![sortCommand](uml/SortAssignmentByDeadlineCommand.png)

Copy link

Choose a reason for hiding this comment

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

Maybe include a short explanation on the user inputs required to start this sorting assignments by deadline logic?

Comment on lines 87 to 91
Step 1. The user launches the application. The CS2113T module has an assignment quiz1 due on 17 Aug 2021.
Step 2. The user adds 2 more assignments quiz2 and quiz3
Step 3. The user only sets the deadline for quiz3 to be 16 Aug 2021.
Step 4. The user executes `sort by deadline` which reorders the assignments in CS2113T to be quiz3, quiz1 and quiz2.
Assignments with null as deadline are sorted behind assignments with deadlines.
Copy link

Choose a reason for hiding this comment

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

Maybe format these steps to be on separate lines? It currently looks like this:
image

Copy link

@hazelhedmine hazelhedmine left a comment

Choose a reason for hiding this comment

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

Most of the features are explained well and supported by class and sequence diagrams! Some minor corrections could improve readability and improve the accuracy of the class and sequence diagrams :)

{Describe the target user profile}

### Value proposition

{Describe the value proposition: what problem does it solve?}
Just to keep track of all informaiton pertaining to a module teaching assistants (at NUS) have to make use of multiple platforms:

Choose a reason for hiding this comment

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

A minor typo for the word "information" here, and perhaps a comma between "module" and "teaching assistants" would increase readability!

<br>
<br>

Given below is the Sequence Diagram for the Storage class. The storage class interacts with the database file, storing our data into the database or loading our data from the database. </br>

Choose a reason for hiding this comment

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

Perhaps this line should be under "Storage Component (Bryan)" instead of "Ui Component"?

<br>
<br>

Given below is the Sequence Diagram for the Storage class. The storage class interacts with the database file, storing our data into the database or loading our data from the database. </br>

Choose a reason for hiding this comment

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

The diagram below is a class diagram, hence a Sequence diagram should be included and/or change the words to mean class diagram.


## Logic Component (Jianning)

![logic](uml/ParserAndCommand.png)

Choose a reason for hiding this comment

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

Perhaps multiplicity should be included in this and the following class diagrams?


Given below is the sequence diagram for the sort assignments by deadline command.

![sortCommand](uml/SortAssignmentByDeadlineCommand.png)

Choose a reason for hiding this comment

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

Perhaps the activation bar for the ":Data" class should have ended upon returning "module", as currently it is activated until the very end.

Copy link

@BlubberMonster BlubberMonster left a comment

Choose a reason for hiding this comment

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

Looks good! Added a few suggestions that can make your dg even better!


Given below is the sequence diagram for the sort assignments by deadline command.

![sortCommand](uml/SortAssignmentByDeadlineCommand.png)

Choose a reason for hiding this comment

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

perhaps can shift the sequence diagram for the reference frame of getCommand down to here.


Given below is the Sequence Diagram for the Storage class. The storage class interacts with the database file, storing our data into the database or loading our data from the database. </br>
## Storage Component (Bryan)
![Storage](uml/StorageClassDiagram.png)

Choose a reason for hiding this comment

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

perhaps for the class diagram, can remove the icon (i.e "C" and "I") as it is not really following the standard convention?


Given below is the sequence diagram for the sort assignments by deadline command.

![sortCommand](uml/SortAssignmentByDeadlineCommand.png)

Choose a reason for hiding this comment

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

perhaps you can remove the object boxes at the bottom of the sequence diagram.

@@ -1,18 +1,120 @@
# Developer Guide

## Design & implementation
## Design

Choose a reason for hiding this comment

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

perhaps can include an introduction to the developer guide.

3. The command execution can affect the state of other components e.g. the sort command changes the order of assignments in `Module`.
4. The `Command` object can also instruct the `Ui` to list and display information to the user.

Given below is the Sequence Diagram for creating the corresponding `Command` object from user input via `Parser`. </br>

Choose a reason for hiding this comment

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

The "</br>" can be fixed, as it is currently visible on the DG. This does not really affect readability too much, but just for the neatness.
image


## Data Component

![Data](uml/Data.png)

Choose a reason for hiding this comment

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

Another point on neatness: maybe the diagrams can be resized such that the font sizes of the DG and the font sizes of the diagram can be consistent? (stated at bottom of https://nus-cs2113-ay2021s2.github.io/website/schedule/week11/tutorial.html)
image


## UI Component
The UI Component consists of one class - `Ui` which provides all the functions
required to print different kinds of messages on te console.

Choose a reason for hiding this comment

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

Small typo: "te" console - but does not affect readability a lot, just for the neatness component.

![Storage](uml/StorageClassDiagram.png)

1. The `Storage` class is used to interact with the database.txt file by calling loadData() at the start of the program and calling saveData() at the end of the program.
2. The `Storage` class will collect the data of all classes that implements the Storable interface so that these data can be saved to the database.

Choose a reason for hiding this comment

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

Since multiple classes are interacting with the Storage class, maybe it would be nice to include 1 small example of a class interacting with the Storage class with a sequence diagram?

4. The `Command` object can also instruct the `Ui` to list and display information to the user.

Given below is the Sequence Diagram for creating the corresponding `Command` object from user input via `Parser`. </br>
The sequence diagram also acts as a reference frame for `getCommand`.

Choose a reason for hiding this comment

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

It could be slightly confusing to a reader as to where /in what context getCommand is actually used, since getCommand is not referenced anywhere else. Perhaps it would be useful to include a brief definition of it?

Copy link

@Chiamjiaen Chiamjiaen left a comment

Choose a reason for hiding this comment

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

image
Some rogue breaks are spotted in this image

Copy link

@Chiamjiaen Chiamjiaen left a comment

Choose a reason for hiding this comment

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

image
This section still seems incomplete

Copy link

@Chiamjiaen Chiamjiaen left a comment

Choose a reason for hiding this comment

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

image
Steps are clumped together making it very hard to view and understand

Copy link

@Chiamjiaen Chiamjiaen left a comment

Choose a reason for hiding this comment

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

image
There are some grammatical errors in the sentences in this image

Copy link

@Chiamjiaen Chiamjiaen left a comment

Choose a reason for hiding this comment

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

Good use of images and indentations! Images are reasonably clear and accurate! Sections are also clearly marked out!

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.

10 participants