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-W13-2] FitnessDuke #53

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

Conversation

EangJS
Copy link

@EangJS EangJS commented Mar 3, 2023

FitnessDuke is a CLI based workout tracker and guider, that helps individuals get them started on proper workout routines on a fixed schedule in the hopes of getting them fit and healthier.

kyrixn added a commit to kyrixn/tp that referenced this pull request Mar 16, 2023
…fix-edit-index

Merge from fix-edit-index to master
Copy link

@Ng-YZ Ng-YZ left a comment

Choose a reason for hiding this comment

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

Easy to read Developer Guide in general, could include slightly more details in some sections though!

Comment on lines 114 to 119
<div align="center">
<img src="UML/Images/CommandHandler.png"/>
<p>
Figure 1.5
</p>
</div>
Copy link

Choose a reason for hiding this comment

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

image
Perhaps can consider using reference frame over ExerciseSessionCommandHandler/GeneralCommandHandler then would also be nice to have separate sequence diagram in the next section(s) to show details.

Comment on lines 128 to 133
<div align="center">
<img src="UML/Images/CommandHandler.png"/>
<p>
Figure 1.6
</p>
</div>
Copy link

Choose a reason for hiding this comment

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

Is this figure meant to be the same as the one above? Would be nice if its a diagram showing how ExerciseSessionCommandHandler relates to the ExerciseStateHandler

Comment on lines 142 to 147
<img src="UML/Images/ErrorMessagesEnum.png"/>
<div align="center">
<p>
Figure 1.7
</p>
</div>
Copy link

Choose a reason for hiding this comment

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

image
Could consider including the enumeration values for ErrorMessages

| V2.0 | - User | - be able to start a workout and set it as complete and save it | - reflect and track my progress |
| V2.0 | - User looking for motivation | - be able to track my workout history as statistics | - better visualise my overall progress |
| V2.0 | - user with little to no experience with exercise | - be given instructions for the specific exercise that I am working on | be educated on how to complete the exercise correctly |
| V2.0 | - User who wants to stay motivated to workout </br> - User who wants to feel good about my past workouts | - See myself be able to accomplish or achieve incrementally greater goals </br> - Keep track of all my exercises | - Continue to stay motivated in making exercise a fun, long-lasting habit |
Copy link

Choose a reason for hiding this comment

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

image
Seems like some slight formatting issue here

Copy link
Author

Choose a reason for hiding this comment

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

Yup, thats a known issue.Will correct them in the future iteration(s) 👍

Comment on lines 142 to 147
<img src="UML/Images/ErrorMessagesEnum.png"/>
<div align="center">
<p>
Figure 1.7
</p>
</div>
Copy link

Choose a reason for hiding this comment

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

image
Not very sure what this diagram is trying to show, composition seems a bit weird?

Copy link

@chinhan99 chinhan99 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 attempt on your diagrams. Do note the syntax errors as discussed ! Thanks!

Choose a reason for hiding this comment

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

Incorrect syntax such as "C" logo and the circle symbols. Do stick to cs2113 syntax

Choose a reason for hiding this comment

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

Check the arrows as discussed

Choose a reason for hiding this comment

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

Do add the termination "cross" if necessary

Choose a reason for hiding this comment

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

Check on the activation bars. and remove the class boxes at the bottom

Choose a reason for hiding this comment

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

Maybe can add more description to explain the way the classes interact with each other.

The class diagram as shown in Figure *1.3* illustrates the structure of the different classes in Ui.

<div align="center">
<img src="UML/Images/Ui.png"/>

Choose a reason for hiding this comment

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

image
For the class diagram, do consider removing the "C" logo in front of the class name and change the visibility to +/-/# instead as it is not the intended styling of in this module

#### Error Message Handling
Enumeration: [```ErrorMessages.java```]
All error messages are stored in the ErrorMessage enumeration for easy access across different classes that could run into similar exceptions.
<img src="UML/Images/ErrorMessagesEnum.png"/>

Choose a reason for hiding this comment

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

image
Do consider changing the dotted solid arrow as I'm not sure we have learnt it in this module, hence readers may be confused by what it means


1. Information messages that describe the functionalities of the program (`Greet` , `Bye`, `PrintExercises` classes)
2. Help messages that details the usage of the possible commands available in the program (`PrintHelpMessage` class)
3. Error messages that trigger when a user's input is incorrect and provides an explanation to the

Choose a reason for hiding this comment

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

Maybe can give some examples of how the error messages are triggered

* Handles when there is no exercise ongoing, granting access to generating new exercises with different confines.

<div align="center">
<img src="UML/Images/CommandHandler.png"/>

Choose a reason for hiding this comment

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

image

Can consider removing the class boxes at the bottom

<div align="center">
<img src="UML/Images/overall_sequence-0.png"/>
<p>
Figure 1.2
Copy link

Choose a reason for hiding this comment

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

It will be good to show the end of class usage using the cross

<div align="center">
<img src="UML/Images/overall_sequence-0.png"/>
<p>
Figure 1.2
Copy link

Choose a reason for hiding this comment

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

It will be good to be consistent with the color coding of the classes. commandHandler and Storage has the same colour


1. Information messages that describe the functionalities of the program (`Greet` , `Bye`, `PrintExercises` classes)
2. Help messages that details the usage of the possible commands available in the program (`PrintHelpMessage` class)
3. Error messages that trigger when a user's input is incorrect and provides an explanation to the
Copy link

Choose a reason for hiding this comment

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

It will be good to show examples of errors

| V2.0 | - User | - be able to start a workout and set it as complete and save it | - reflect and track my progress |
| V2.0 | - User looking for motivation | - be able to track my workout history as statistics | - better visualise my overall progress |
| V2.0 | - user with little to no experience with exercise | - be given instructions for the specific exercise that I am working on | be educated on how to complete the exercise correctly |
| V2.0 | - User who wants to stay motivated to workout </br> - User who wants to feel good about my past workouts | - See myself be able to accomplish or achieve incrementally greater goals </br> - Keep track of all my exercises | - Continue to stay motivated in making exercise a fun, long-lasting habit |
Copy link

Choose a reason for hiding this comment

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

There is a formatting issue here

Test cases: ```o``` , ```hi```
Expected: Error details will be shown in the terminal
### ```find``` command
1. Find a set of exercises based on a specified keyword : ```find [keyword]```
Copy link

Choose a reason for hiding this comment

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

It will be good to show an example to the users

<div align="center">
<img src="UML/Images/Storage.png"/>
<p>
Figure 1.4
Copy link

Choose a reason for hiding this comment

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

There can be more comprehensive explanation to the diagram given the diagram's complexity

<div align="center">
<img src="UML/Images/Ui.png"/>
<p>
Figure 1.3
Copy link

Choose a reason for hiding this comment

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

There can be more comprehensive explanation to the diagram given the diagram's complexity

<div align="center">
<img src="UML/Images/architecture.png"/>
<p>
Figure 1.1
Copy link

Choose a reason for hiding this comment

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

Overall comments on the diagrams are that there can be more consistency in the colors across the different diagrams. Some of the diagrams have colours and some don't. It does not seem like it is coded by one person. Also, given that users read the DG for the first time, diagrams, even seemingly simple diagrams can be complicated. It will be good to have more explanations

Expected: Error details will be shown in the terminal
### ```find``` command
1. Find a set of exercises based on a specified keyword : ```find [keyword]```
2. Test case: ```find```
Copy link

Choose a reason for hiding this comment

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

It will be good to add additional invalid entries of your commands


{Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing}
---
title: Developer Guide
Copy link

Choose a reason for hiding this comment

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

I like that the DG is very organized and the Dg is filled with diagram that is adequately understandable. Keep up the good work.

Magmanat

This comment was marked as outdated.

Copy link

@Magmanat Magmanat left a comment

Choose a reason for hiding this comment

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

Overall Neat Developer Documentation, however could have better clarity on some of the diagrams

Comment on lines +22 to +27
<div align="center">
<img src="UML/Images/architecture.png"/>
<p>
Figure 1.1
</p>
</div>

Choose a reason for hiding this comment

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

Not too sure why there are dotted arrows and solid arrows in the diagram, could be clearer in explaining the difference as developers might not understand the meaning behind them

image

Comment on lines 43 to 48
<div align="center">
<img src="UML/Images/overall_sequence-0.png"/>
<p>
Figure 1.2
</p>
</div>

Choose a reason for hiding this comment

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

This is a nice diagram which is segregated by colors to represent the classes for easy reference, however this is not consistent with the rest of the developers docs, consider updating the rest of the dev docs to follow this format

Comment on lines 95 to 101
The class diagram as shown in *Figure 1.4* illustrates the structure of the different classes in Storage.
<div align="center">
<img src="UML/Images/Storage.png"/>
<p>
Figure 1.4
</p>
</div>

Choose a reason for hiding this comment

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

Diagram looks neat, however Storage Inherits from two different interfaces, is this possible in Java?
image

Copy link
Author

Choose a reason for hiding this comment

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

Storage interface extends UserPlansStorsge and UserCareerStorage interfaces. This is similar to AB3.

Comment on lines 95 to 101
The class diagram as shown in *Figure 1.4* illustrates the structure of the different classes in Storage.
<div align="center">
<img src="UML/Images/Storage.png"/>
<p>
Figure 1.4
</p>
</div>

Choose a reason for hiding this comment

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

The StorageManager has an association with UserPlansStorage and UserCareerStorage, however this is might be already implied and not needed since StorageManager implements Storage which inherits from UserPlansStorage and UserCareerStorage.

The StorageManager has an association with UserPlansStorage and UserCareerStorage, however this is might be already implied and not needed since StorageManager implements Storage which inherits from UserPlansStorage and UserCareerStorage.

Comment on lines 148 to 151
#### Error Message Handling
Enumeration: [```ErrorMessages.java```]
All error messages are stored in the ErrorMessage enumeration for easy access across different classes that could run into similar exceptions.
<img src="UML/Images/ErrorMessagesEnum.png"/>

Choose a reason for hiding this comment

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

The dotted line with the black diamond is not a valid uml arrow representation which we have learnt in the module, possibly you meant to use a hollow diamond instead to represent aggregation?

image

Comment on lines 135 to 142
To manage this exists the ```ExerciseStateHandler```, which allows for saving, starting
ending, cancelling workouts.
<div align="center">
<img src="UML/Images/CommandHandler.png"/>
<p>
Figure 1.7
</p>
</div>

Choose a reason for hiding this comment

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

Possibly missing out on the activation bar for all the classes that are involved in this sequential diagram.

image

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