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-F12-3] Teaching Assistant's Assistant #46

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

Conversation

hozhenhong99
Copy link

Teaching Assistant's Assistant helps teaching assistants manage details of their students. It is optimised for CLI users so that managing information can be done faster by typing in commands.

Copy link

@Rakesh12000 Rakesh12000 left a comment

Choose a reason for hiding this comment

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

DG Review

* Contains a list of `Module` objects currently being managed by the app.
* `Util`
* Contains useful methods (e.g. Check if a string is integer/double, convert string to integer/double).
* `Storage`

Choose a reason for hiding this comment

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

Youd could add UML diagrams for the Storage function. This will enable your users to understand how data is written and stored by TAA as well as how it is accessed an read as inputs.


<br>

### Add Assessment

Choose a reason for hiding this comment

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

The description for the implementation of Assessment tends to be a bit wordy and hard to follow. To make it easier for your users to understand this command function you could add an UML sequence diagram like the other commands.


<br>

### Set Marks

Choose a reason for hiding this comment

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

You could add an example scenario like the other commands so that it is easier for your users to understand how this command works.

* Defines how a command is to be executed.
* `ModuleList`
* Contains a list of `Module` objects currently being managed by the app.
* `Util`

Choose a reason for hiding this comment

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

You could further explain in-depth how the util component works. This will help your users get an idea of what util can be used for when they work on your code.

9. A message will then be printed out to indicate to the user that the marks have been set successfully.


### Set Attendance

Choose a reason for hiding this comment

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

You could use an UML sequence diagram here as well for easier reference of how this command function works for your users.

* `StudentList#findStudent(keyword:String)` - Returns an ArrayList of students containing the keyword

The sequence diagram shown below illustrates how the `add_student` command works:
![AddStudentSequenceDiagram](diagrams/AddStudentSequenceDiagram.png)

Choose a reason for hiding this comment

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

You could maybe add numbering in the diagram so that you can link the steps to the diagram to make it more clear.

Choose a reason for hiding this comment

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

Perhaps you could explain step by step how the add_student command is being parsed and saved.

Choose a reason for hiding this comment

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

You could perhaps split up the uml diagram to include sub parts of the e.g. focuses on the logic only so that the uml diagram could be bigger and clearer. ref: https://se-education.org/addressbook-level3/DeveloperGuide.html

## Implementation
### Add Module
The sequence diagram shown below illustrates how the `add_module` command works:
![AddModuleSequenceDiagram](diagrams/AddModuleSequenceDiagram.png)

Choose a reason for hiding this comment

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

Nice description for the steps, but maybe you can add numbering in the diagram and link it to the steps.


### Set Marks
The sequence diagram shown below illustrates how the `set_mark` command works:
![SetMarkSequenceDiagram](diagrams/SetMarkSequenceDiagram.png)

Choose a reason for hiding this comment

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

You could maybe also add numbering for the diagram so that readers can easily refer to the diagram from the steps.

* `AttendanceList#sortAttendances` - Sorts the attendance in the `attendances` ArrayList in ascending order based on
lesson number.

Below is an example scenario of how the set attendance feature behaves at each step:

Choose a reason for hiding this comment

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

The steps here look a bit chunky, maybe you could split them up into more steps which could help readers digest it better.

Copy link

@conradwee conradwee left a comment

Choose a reason for hiding this comment

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

this looks like a great job, just make some minor changes and alls good

@@ -1,38 +1,228 @@
# Developer Guide

Choose a reason for hiding this comment

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

great work!


## Design
### Architecture
![ArchitectureDiagram](diagrams/ArchitectureDiagram.png)

Choose a reason for hiding this comment

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

would it be better to use UML here?


<br>

### Add Assessment

Choose a reason for hiding this comment

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

Neatness/correctness:
looks messy
not well-formatted
hard to read/understand


<br>

### Set Marks

Choose a reason for hiding this comment

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

Use of examples:

Not enough or too many examples e.g., sample inputs/outputs

lesson number.

Below is an example scenario of how the set attendance feature behaves at each step:
* Step 1 - The user executes `set_attendance c/CS2113T s/1 l/1 p/1` to set an attendance to `Present` for student at

Choose a reason for hiding this comment

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

Neatness/correctness:
looks messy
not well-formatted
hard to read/understand

perhaps some more spacing at a minimum

Copy link

@Nirmaleshwar Nirmaleshwar left a comment

Choose a reason for hiding this comment

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

Overall a very good job, just a few minor changes but since it's a work in progress, it's well on track.

Comment on lines -36 to -38
## Instructions for manual testing

{Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing}

Choose a reason for hiding this comment

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

I'm not sure if you're going you add this later but, perhaps adding some simple manual testing directions can guide the person to test functions you've described so far.

Comment on lines 163 to 172
`AttendanceList` implements the following methods:
* `AttendanceList#getSize()` - Returns the no. of attendance currently in the list.
* `AttendanceList#getAttendances()` - Returns an ArrayList containing all the attendances.
* `AttendanceList#getAttendance(lessonNumber:String)` - Returns an attendance with a particular lesson number.
* `AttendanceList#isValidIndex(index:int)` - Checks if an index is valid w.r.t the `attendances` ArrayList.
* `AttendanceList#addAttendance(attendance:Attendance)` - Adds an attendance to the `attendances` ArrayList.
* `AttendnaceList#getAttendnaceIndex(lessonNumInput:String)` - Returns the attendance index in the `attendances`
ArrayList.
* `AttendanceList#deleteAttendance(lessonNumInput:String)` - Deletes an attendance with a particular lesson number.
* `AttendanceList#sortAttendances` - Sorts the attendance in the `attendances` ArrayList in ascending order based on

Choose a reason for hiding this comment

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

Maybe can consider putting these on a table as they are a bit too hard to read.

* On app launch: Creates and runs an instance of `Taa`.
* `UI`
* Handles UI operations.
* `Taa`

Choose a reason for hiding this comment

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

The sequence diagram of TAA has a problem:

image

The activation bar of Taa should be closed

* On run: Loads persistent data from `Storage`, receives user input from `UI`, and uses `Parser` to parse the user input.
* `Parser`
* Handles input parsing and determines which command to run.
* `Command`

Choose a reason for hiding this comment

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

Would it be better to add function notation?

image

Choose a reason for hiding this comment

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

Better to add function notation
image

<br>

**Interaction between components**
![ArchitectureSequenceDiagram](diagrams/ArchitectureSequenceDiagram.png)

Choose a reason for hiding this comment

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

It would be better to add the notation of the grey file icon
image

Copy link

@tttyyzzz tttyyzzz 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 uml diagrams with detailed labels. Could use numbering in uml diagrams if it helps in explanation. Perhaps standardise the steps underneath the uml diagrams. Good Job!

* `Util`
* Contains useful methods (e.g. Check if a string is integer/double, convert string to integer/double).
* `Storage`
* Handles data storage operations (e.g. Reading from and writing to data file).

Choose a reason for hiding this comment

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

image
Perhaps adding the new ModuleList()

Choose a reason for hiding this comment

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

image
Perhaps adding new Command() upon instantiation

from the `assessments` ArrayList.

The sequence diagram shown below illustrates how the `add_assessment` command works:
![AddAssessmentSequenceDiagram](diagrams/AddAssessmentSequenceDiagram.png)

Choose a reason for hiding this comment

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

image
Perhaps adding the 'X' below the addModuleCommand

## Implementation
### Add Module
The sequence diagram shown below illustrates how the `add_module` command works:
![AddModuleSequenceDiagram](diagrams/AddModuleSequenceDiagram.png)

Choose a reason for hiding this comment

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

image
Perhaps deleting the redundant class.

* `StudentList#findStudent(keyword:String)` - Returns an ArrayList of students containing the keyword

The sequence diagram shown below illustrates how the `add_student` command works:
![AddStudentSequenceDiagram](diagrams/AddStudentSequenceDiagram.png)

Choose a reason for hiding this comment

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

Perhaps you could explain step by step how the add_student command is being parsed and saved.

* `StudentList#findStudent(keyword:String)` - Returns an ArrayList of students containing the keyword

The sequence diagram shown below illustrates how the `add_student` command works:
![AddStudentSequenceDiagram](diagrams/AddStudentSequenceDiagram.png)

Choose a reason for hiding this comment

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

You could perhaps split up the uml diagram to include sub parts of the e.g. focuses on the logic only so that the uml diagram could be bigger and clearer. ref: https://se-education.org/addressbook-level3/DeveloperGuide.html

rachelkeh and others added 24 commits November 6, 2021 19:02
Change type of assessment from int to double and update all the strin…
Update TaaClassDiagram and CommandClassDiagram
Change maximumMarks in AssessmentDeserializer to double and check if student comment is valid in StudentDeserializer
Update comparison of doubles to a higher precision with Double.compare
jon-the-melon and others added 30 commits November 8, 2021 18:24
add object diagram for add student command
update dg contributions in ppp
modify verify in attendance
update dg with new sequence diagrams
Check for decimal places in deserializers and add more tests
…ementation

add header to attendance implementation
Add class diagram for assessment list and update sequence diagram for…
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.