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-F14-3] CLI.ckFit #51

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

Conversation

VishalJeyaram
Copy link

CLI.ckFit is a desktop app for managing your nutrition and fitness needs via a Command Line Interface (CLI). It allows you to track your meals, recipes, calories, water intake and exercise regimen conveniently.

Copy link

@rondayvoo rondayvoo 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 well-documented and diagrammed developer guide. Keep it up!


#### Meal: Class Diagram

![](https://user-images.githubusercontent.com/69350459/138307467-cef8cdd8-06ce-4284-92b5-9fe5e1ef50ef.png)

Choose a reason for hiding this comment

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

I feel like having the class diagrams before the sequence diagrams would make more sense.


## Design & implementation
## Ui(User Interface): Class diagram

Choose a reason for hiding this comment

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

I feel that the "Design and Implementation" header should have a larger size than the "Ui(User Interface): Class diagram" header as it encompasses the rest of the fields.

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

Choose a reason for hiding this comment

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

I feel that a table of contents would greatly help in my navigation of this guide.

Copy link

Choose a reason for hiding this comment

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

Agreed. Also, perhaps fix a standard for the heading size to better the hiearchy between the sections? This is because I observed that the headings for class diagrams are of different size (e.g Ui(User Interface): Class diagram and WeightTracker: Class diagram)

Above are the UML class level diagrams of `Fluid`, `FluidExceptions` and `Tracker`. As seen in the diagram, the `Fluid` class is dependent on the `FluidExceptions` and the `Fluid`class inherits from the `Tracker` class. This class diagram has been simplified for better readability.

#### Fluid: Adding weight sequence diagram
![](https://user-images.githubusercontent.com/69446495/138881867-cff1b0a7-e836-43dd-b654-5b7db96ea1a3.png)

Choose a reason for hiding this comment

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

I think you can omit a lot of the attributes and methods from your class diagram to make it simpler.

Copy link

@KaiserHuang88 KaiserHuang88 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


### Meal: Listing Meals
![](https://user-images.githubusercontent.com/69350459/138880611-c82f4574-037f-4b64-9631-90d914f71701.png)

Choose a reason for hiding this comment

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

Maybe a brief description of the Listing Meals Sequence Diagram can be added? For standardisation purposes since all the other sequence diagrams have brief descriptions, and also for clarification purposes since it would help explain the diagram better.

This class diagram has been simplified for better readability.

#### WeightTracker: Adding weight
![WeightTracker_add_sequence](https://user-images.githubusercontent.com/69446729/138879720-3c3632c5-0765-4215-a2f8-5df7eea45277.png)

Choose a reason for hiding this comment

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

Maybe the parameters of each function can be omitted in the sequence diagram? (This comment applies to all subsequent sequence diagrams). The reason being that it makes the diagram look less cluttered whilst also retaining enough important information

Copy link

Choose a reason for hiding this comment

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

Also, perhaps replace the weightTracker.addWeights(...) with addWeights(...)? Since the arrow is pointing at the WeightTracker we know that the method belongs to this class.

when an exception is encountered, the `WeightTracker` class will throw `AddWeightException()` instead.

### ScheduleTracker: Class diagram
![diagram-2070120484733536202](https://user-images.githubusercontent.com/69461398/138324203-ea286780-6611-43f4-af77-3ea7cb59a42c.png)

Choose a reason for hiding this comment

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

Maybe can standardise operator symbols ('#', (+), (-) is each class diagram to make reading of each diagram smoother?

daily caloric intake through getIdealCalories().

### Get summary of all info stored in text files
The user is next greeted with a second prompt that asks whether he or she wishes to get a summary of all meals, fluids

Choose a reason for hiding this comment

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

Perhaps this part can have a class / sequence diagram?

Copy link

@alwinangys alwinangys left a comment

Choose a reason for hiding this comment

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

Good descriptions and diagrams, just that some diagrams require minor changes.


#### Fluid: Class diagram

![](https://user-images.githubusercontent.com/69446495/138308110-c73bc021-3744-4164-98dc-52b7f76cb4c0.png)

Choose a reason for hiding this comment

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

dg2

While it is correct that Fluid extends Tracker, does FluidExceptions extend Fluid? Perhaps you meant to show another relationship between Fluid and FluidExceptions instea

### Printing the welcome message
The user launches the CLI for the first time. The welcomeMessage() is called first and prints out the messages imported from Clickfitmessages class.

### Getting BMI and recommended daily caloric intake

Choose a reason for hiding this comment

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

dg-1

Perhaps you could consider turning off the icons from the class names.
A suggestion to include in the .puml file could be:

hide circle
skinparam classAttributeIconSize 0 as suggested by Prof Akshay:
nus-cs2113-AY2122S1/forum#11

Above are the UML class level diagrams of `Fluid`, `FluidExceptions` and `Tracker`. As seen in the diagram, the `Fluid` class is dependent on the `FluidExceptions` and the `Fluid`class inherits from the `Tracker` class. This class diagram has been simplified for better readability.

#### Fluid: Adding weight sequence diagram
![](https://user-images.githubusercontent.com/69446495/138881867-cff1b0a7-e836-43dd-b654-5b7db96ea1a3.png)

Choose a reason for hiding this comment

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

dg3

As Parser calls addFluid from Fluid, perhaps there could be a dotted arrow at the end of the activation block (in opposite direction as addFluid) for the Fluid object to show that the control is given back to Parser?

This "updating" is done in any method call that outputs something to the user to ensure a correctly sorted and cleaned up
list is always output to the user. This also ensures the `scheduledWorkouts` ArrayList remains free of overdue workouts.

#### Meal: Class Diagram

Choose a reason for hiding this comment

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

dg4

Perhaps the visibility of attributes could be changed to using +, -, #, ~ instead as it is not explicit in this class diagram. However, other diagrams are correct, which is good to see!

Copy link

Choose a reason for hiding this comment

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

Agreed. Also, perhaps change the protected ArrayList<String> meals; to # meals: ArrayList<String>? I could probably tell that this line was copied directly from the actual code.

image

when an exception is encountered, the `WeightTracker` class will throw `AddWeightException()` instead.

### ScheduleTracker: Class diagram
![diagram-2070120484733536202](https://user-images.githubusercontent.com/69461398/138324203-ea286780-6611-43f4-af77-3ea7cb59a42c.png)

Choose a reason for hiding this comment

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

dg5

Perhaps the private in private String activityDescription could be changed to -activityDescription: String instead.


## Design & implementation

Choose a reason for hiding this comment

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

Maybe an general introduction to the product to give some context, instead of diving directly into the different classes


{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
### Design & implementation

Choose a reason for hiding this comment

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

Might be good to provide a high level class/sequence diagram of the overall product and how the different classes interact with each other



### Meal: Listing Meals
![](https://user-images.githubusercontent.com/69350459/138880611-c82f4574-037f-4b64-9631-90d914f71701.png)

Choose a reason for hiding this comment

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

Screenshot 2021-10-28 at 9 38 28 AM

Maybe the alt, loop and opt branches can have a background fill of different colours to better differentiate them and make the class diagram easier to understand

Copy link

Choose a reason for hiding this comment

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

Agreed. Also, perhaps add hide footbox to your PUML code for your sequence diagrams so the bottom of the lifeline should not have the meal: Meal box and the lifeline (the dotted line) will continue extending down?

This "updating" is done in any method call that outputs something to the user to ensure a correctly sorted and cleaned up
list is always output to the user. This also ensures the `scheduledWorkouts` ArrayList remains free of overdue workouts.

#### Meal: Class Diagram

Choose a reason for hiding this comment

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

The Meal class diagram should not need to have extends since inheritance is already shown through the triangular arrow?

Copy link

@rizemon rizemon 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

Comment on lines 48 to 49
![WeightTracker_class](https://user-images.githubusercontent.com/69446729/138873653-d5db5c99-1f22-4c68-86af-188f1ea2c593.png)

Copy link

Choose a reason for hiding this comment

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

The wordings here are quite small and very difficult to read so perhaps resize the image to make it more readable?

Copy link

Choose a reason for hiding this comment

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

Perhaps move the exceptions into its own class diagram? I feel that the exceptions here feels kind of out of place and doesn't aid in allowing readers to better understand.



### Meal: Listing Meals
![](https://user-images.githubusercontent.com/69350459/138880611-c82f4574-037f-4b64-9631-90d914f71701.png)
Copy link

Choose a reason for hiding this comment

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

Agreed. Also, perhaps add hide footbox to your PUML code for your sequence diagrams so the bottom of the lifeline should not have the meal: Meal box and the lifeline (the dotted line) will continue extending down?

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

Copy link

Choose a reason for hiding this comment

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

Agreed. Also, perhaps fix a standard for the heading size to better the hiearchy between the sections? This is because I observed that the headings for class diagrams are of different size (e.g Ui(User Interface): Class diagram and WeightTracker: Class diagram)

This "updating" is done in any method call that outputs something to the user to ensure a correctly sorted and cleaned up
list is always output to the user. This also ensures the `scheduledWorkouts` ArrayList remains free of overdue workouts.

#### Meal: Class Diagram
Copy link

Choose a reason for hiding this comment

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

Agreed. Also, perhaps change the protected ArrayList<String> meals; to # meals: ArrayList<String>? I could probably tell that this line was copied directly from the actual code.

image

This class diagram has been simplified for better readability.

#### WeightTracker: Adding weight
![WeightTracker_add_sequence](https://user-images.githubusercontent.com/69446729/138879720-3c3632c5-0765-4215-a2f8-5df7eea45277.png)
Copy link

Choose a reason for hiding this comment

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

Also, perhaps replace the weightTracker.addWeights(...) with addWeights(...)? Since the arrow is pointing at the WeightTracker we know that the method belongs to this class.

@silinche
Copy link

image
Here for the sequence diagram list meal, is it possible that a condition is missing for the "alt"

@silinche
Copy link

image
Sequence diagram for adding scheduled workout might be a bit complex? Perhaps use ref in this case?



### Meal: Listing Meals
![](https://user-images.githubusercontent.com/69350459/138880611-c82f4574-037f-4b64-9631-90d914f71701.png)
Copy link

Choose a reason for hiding this comment

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

for alt, box below should have a condition as well



### WeightTracker: Class diagram
![WeightTracker_class](https://user-images.githubusercontent.com/69446729/138873653-d5db5c99-1f22-4c68-86af-188f1ea2c593.png)
Copy link

Choose a reason for hiding this comment

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

Perhaps class diagram could be more comprehensible rather than comprehensive, you could possibly omit less important details


#### ScheduleTracker: Adding scheduled workout

![diagram-18374474381804594155](https://user-images.githubusercontent.com/69461398/138323717-0975d9b3-392a-4c41-99c4-73a8915933be.png)
Copy link

Choose a reason for hiding this comment

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

Quite well done and easy to understand sequence diagram

printed as a summary of all stored information iu the text files.


### Meal: Listing Meals
Copy link

Choose a reason for hiding this comment

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

Should this be shifted to below meal class diagram instead? Since all other classes have a class and sequence diagram.

teoziyiivy and others added 30 commits November 8, 2021 20:39
DG updates + Fluid bug fixes
# Conflicts:
#	diagrams/ScheduleTracker_class.puml
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.