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

[CS2103T-W17-4] TA Tracker #8

Open
wants to merge 1,743 commits into
base: master
Choose a base branch
from

Conversation

PotatoCombat
Copy link

Here are the usernames of our team members:

@Eclmist
@aakanksha-rai
@chuayijing
@fatin99
@PotatoCombat

Copy link
Contributor

@j-lum j-lum left a comment

Choose a reason for hiding this comment

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

Commit message title doesn't match content of commit.

Copy link

@truevchow truevchow left a comment

Choose a reason for hiding this comment

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

The image of Figure 3 is not updated, even though the .puml file is.
The images in section 3.1.1 are not updated or labelled.

Comment on lines 419 to 421
1. LogicManager uses the TATrackerParser to first parse the user command.

2. The TATrackerParser sees that the command is of type sort and passes the

Choose a reason for hiding this comment

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

TATrackerParser should be in camel case to be consistent with the class name.


The following sequence diagram shows the sequence of commands that take place
between the logic and model components of the TA-Tracker when the user enters the
command 'module add m/CS2103 n/Software Engineering'.

Choose a reason for hiding this comment

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

Try to denote quoted commands with different formatting from the main text.

@Capeguy
Copy link

Capeguy commented Mar 25, 2020

Feedback for W17-4 DG

  1. Design
    a. Figure 1 is incomplete
    b. Figure 7 seems excessively large
    c. There are figures after Figure 7 which are unlabelled.

  2. Implementation
    a. Many unlabelled figures
    b. Can use 'code font' for words such as "UniqueModuleList"
    c. Figure 10 in 3.2.2. too small
    d. Figure 12 in 3.2.3. too small
    e. Figure 17 in 3.3.2. too small
    f. Figure 19 in 3.3.3. too small
    g. Figures in 3.8 does not follow numbering and numbering formatting
    h. Figure '2' in 3.8.1 has black font on red background (hard to read)

@umaikaze
Copy link

umaikaze commented Mar 25, 2020

Feedback for W17-4 DG

Very little use of IN_LINE_CODE to highlight the in-line code in the first half of the DG, might want to consider using them to increase readability. e.g. TotalEarnings is calculated by multiplying TotalHours with Rate

@@ -1,4 +1,4 @@
= AddressBook Level 3 - Developer Guide
= TA-Tracker - Developer Guide

Choose a reason for hiding this comment

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

Your UML diagrams can be less cluttered

Choose a reason for hiding this comment

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

Especially the sequence diagrams

@@ -96,11 +96,16 @@ The `UI` component,
image::LogicClassDiagram.png[]

Choose a reason for hiding this comment

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

Maybe can modify the diagram above so that these 2 diagrams don't repeat each other??

.Group Delete - Sequence Diagram
image::DeleteGroupSequenceDiagram.png[]

1. LogicManager uses the TATrackerParser to first parse the user command.

Choose a reason for hiding this comment

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

Words such as LogicManager an be formatted to look nicer:)

class structure is used.

.Sort Commands - Class Diagram
image::SortCommandsClassDiagram.png[]

Choose a reason for hiding this comment

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

The words here are a little small to see (compared to the other diagrams) so maybe can change the sizing of the words!

PotatoCombat and others added 30 commits April 13, 2020 22:25
Fix editing sessions with invalid module code
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.