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-W10-3] BuffBuddy #20

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

Conversation

TVageesan
Copy link

BuffBuddy aims to help people achieve their fitness goals by tracking their activities, calories, and water intake. It also provides helpful suggestions and information to provide a all-in-one fitness solution app, with a CLI interface to make accessing said information as quick as possible.

Copy link

@joshuan98 joshuan98 left a comment

Choose a reason for hiding this comment

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

There are many instances of magic strings and inconsistent formatting. Please consider taking a look and resolve these.


public Day deleteDay(int index){
if (dayList.size() < index){
System.out.println("invalid index");

Choose a reason for hiding this comment

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

You can consider using constant field for error messages instead of a magic string

Comment on lines 68 to 73
private JsonObject createJSON(ProgrammeList programmeList, History history) {
JsonObject jsonObject = new JsonObject();
jsonObject.add("programmeList", programmeList.toJson());
jsonObject.add("history", history.toJson());
return jsonObject;
}

Choose a reason for hiding this comment

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

Storage seems to handle both file updates and json parsing. You can consider splitting this into a different file.


private JsonObject load() {
try (FileReader reader = new FileReader(path)){
JsonElement elememt = JsonParser.parseReader(reader);

Choose a reason for hiding this comment

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

Do remember to check for spelling mistakes

import parser.Parser;
import programme.ProgrammeList;

public class BuffBuddy {

Choose a reason for hiding this comment

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

This class seems to be handling multiple functions. You can consider splitting it into different files.

public class CreateCommand extends Command {
public static final String COMMAND_WORD = "create";
private final String name;
private final ArrayList<Day> contents;

Choose a reason for hiding this comment

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

Perhaps this naming can be improved.


import static parser.ParserUtils.parseIndex;

public class Parser {

Choose a reason for hiding this comment

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

Do remember to add relevant code comments

case LogCommand.COMMAND_WORD -> prepareLogCommand(argumentString);
case HistoryCommand.COMMAND_WORD -> new HistoryCommand();
case ExitCommand.COMMAND_WORD -> new ExitCommand();
default -> new InvalidCommand();

Choose a reason for hiding this comment

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

I think you should standardise how you write your switch statements.

Comment on lines 53 to 64
switch (flag){
case "/p":
progIndex = parseIndex(value);
break;
case "/d":
dayIndex = parseIndex(value);
break;
case "/t":
date = parseDate(value);
break;
default:
throw new IllegalArgumentException("Flag command not recognized: " + flag);

Choose a reason for hiding this comment

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

This may be considered as nesting by some. You can consider refactoring this.

Comment on lines 9 to 11
public void assertCommand() {
assertTrue(true);
}

Choose a reason for hiding this comment

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

Not too sure about the purpose of this test.

## Design & implementation
### DailyRecord component
API: `DailyRecord.java`
![Diagram for DailyRecord Component](./images/DailyRecord_API_UML.jpg)

Choose a reason for hiding this comment

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

You could omit methods that are not directly relevant. Such as the toString() method in the DailyRecord UML diagram

Choose a reason for hiding this comment

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

Navigability is missing from Day, MealList and Water class

Choose a reason for hiding this comment

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

Uploading image.png…


### Sequence Diagram

![Sequence Diagram for WeeklySummary feature](./images/History%20WeeklySummary%20UML%20Sequence%20Diagram.png)

Choose a reason for hiding this comment

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

Maybe you could keep the application used to create the UML diagrams consistent?

Choose a reason for hiding this comment

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

image
image


#### Sequence Diagram for "Add Meal" Command

![Add Meal Sequence Diagram](images/AddMeal_Sequence_diagram.png)

Choose a reason for hiding this comment

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

image
The add meal sequence diagram is missing an activation bar in MealList

Copy link

@ehz0ah ehz0ah left a comment

Choose a reason for hiding this comment

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

Overall pretty decent, just a few nits to fix. I am also wondering if the diagram is hand-drawn (on PlantUML or not). If it is, maybe you can use a standardized way to draw them

## Design & implementation
### DailyRecord component
API: `DailyRecord.java`
![Diagram for DailyRecord Component](./images/DailyRecord_API_UML.jpg)
Copy link

Choose a reason for hiding this comment

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

Functions/Methods parameter could include variable name followed by its type for better readability.

Suggested change
![Diagram for DailyRecord Component](./images/DailyRecord_API_UML.jpg)
+ logDay(newDay : Day)


## Design & implementation
### DailyRecord component
API: `DailyRecord.java`
Copy link

Choose a reason for hiding this comment

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

Clear separation of association and dependencies!

objects, restoring the user's previous session.

The following sequence diagram shows how a load operation for ProgrammeList goes through the Storage component:
![Sequence Diagram for Load operation](./images/LoadProgrammeList_Seq_Dia.jpg)
Copy link

Choose a reason for hiding this comment

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

LGTM!

![Sequence Diagram for Load operation](./images/LoadProgrammeList_Seq_Dia.jpg)

The following sequence diagram shows how a save operation goes through the Storage component:
![Sequence Diagram for Save operation](./images/Save_Seq-Dia.jpg)
Copy link

Choose a reason for hiding this comment

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

For both addition of programmeListJsonObject and historyJsonObject using the add() function, you could be more specific and mention it as the arguments of the function.

Suggested change
![Sequence Diagram for Save operation](./images/Save_Seq-Dia.jpg)
add("programmeList", programmeListToJson(programmeList))

Since the add method depends on the Json object, you should consider adding it.
Since the add function depends on the Json object


### Sequence Diagram

![Sequence Diagram for WeeklySummary feature](./images/History%20WeeklySummary%20UML%20Sequence%20Diagram.png)
Copy link

Choose a reason for hiding this comment

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

I think you can be more consistent in your overall DG functions call in terms of how you represent your method's arguments. Some methods are shown where the arguments are represented by their type while some other methods are represented by the actual arguments.


Step 2. The user executes `programme edit /p 1 /d 1 /x 1` to delete the first exercise in the first day of the first programme. The programme first retrieves the given day with `ProgrammeList#getDay()`.

![](images/editCommandStepTwo.png)
Copy link

Choose a reason for hiding this comment

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

This seems slightly too nested to understand 😵

Comment on lines 58 to 76
### Save/Load Feature

The save/load mechanism is handled by three main components: `Storage`, `FileManager`, and `DateSerializer`. `FileManager` manages file interactions, including reading from and writing to JSON data files, while `Storage` handles the conversion between JSON objects and `ProgrammeList`/`History` objects. The `DateSerializer` is used for converting `LocalDate` to/from JSON format.

#### FileManager implements the following key operations:
- **`FileManager#load()`**: Reads the data file into a `JsonObject`.
- **`FileManager#save(JsonObject data)`**: Writes the `JsonObject` containing all data back into the file.
- **`FileManager#createDirIfNotExist()`** and **`FileManager#createFileIfNotExist()`**: Ensure that the necessary directory and file exist before saving.

#### Storage converts JSON data into Java objects:
- **`Storage#loadProgrammeList()`**: Converts the `ProgrammeList` JSON data into a `ProgrammeList` object using `programmeListFromJson()`.
- **`Storage#loadHistory()`**: Converts the `History` JSON data into a `History` object using `historyFromJson()`.
- **`Storage#saveData()`**: Converts `ProgrammeList` and `History` into JSON using `createJSON()`, and passes it to `FileManager#save()`.

#### DateSerializer is responsible for:
- **`DateSerializer#serialize()`**: Converts `LocalDate` into `JsonElement`.
- **`DateSerializer#deserialize()`**: Converts `JsonElement` back into `LocalDate`.

These operations are exposed in the `Model` interface, allowing seamless saving and loading of `ProgrammeList` and `History` data.
Copy link

Choose a reason for hiding this comment

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

Save/Lead implementation overview is accurate and concise. LGTM!

Comment on lines 222 to 234
![Add Meal Step 1](images/AddMeal_Step_one_diagram.png)

**Step 2**: The command retrieves the `DailyRecord` for the specified date from the `History` using `History#getRecordByDate()`. If no record exists, a new one is created.

![Add Meal Step 2](images/AddMeal_Step_two_diagram.png)

**Step 3**: The `AddMealCommand` adds the meal to the `MealList` of the `DailyRecord`. If the meallist already exists, it updates the existing meallist instead.

![Add Meal Step 3](images/AddMeal_Step_three_diagram.png)

**Step 4**: The newly added `Meal` object is returned to the `AddMealCommand` to display as part of the `CommandResult`.

![Add Meal Step 4](images/AddMeal_Step_four_diagram.png)
Copy link

Choose a reason for hiding this comment

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

Should these diagrams be using non-orthogonal lines?

## Design & implementation
### DailyRecord component
API: `DailyRecord.java`
![Diagram for DailyRecord Component](./images/DailyRecord_API_UML.jpg)
Copy link

Choose a reason for hiding this comment

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

You may want to digitise or clarify the names of some of your operations, they are a bit difficult to read at points.

image

Copy link

Choose a reason for hiding this comment

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

Great work! Maybe can use consistent formatting for all diagrams so that readability can be improved and the DG can look more unified overall.


### Sequence Diagram

![Sequence Diagram for WeeklySummary feature](./images/History%20WeeklySummary%20UML%20Sequence%20Diagram.png)
Copy link

Choose a reason for hiding this comment

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

You may want to rewrite / clarify the names used in this diagram, they are a bit difficult to understand. Also nitpicky - but the lines in this diagram (activation bars) are not straight.

image

The following sequence diagram shows how a load operation for ProgrammeList goes through the Storage component:
![Sequence Diagram for Load operation](./images/LoadProgrammeList_Seq_Dia.jpg)

The following sequence diagram shows how a save operation goes through the Storage component:
Copy link

Choose a reason for hiding this comment

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

You may want to rewrite operation names, they are unclear and very difficult to understand currently.

image

![Sequence Diagram for Load operation](./images/LoadProgrammeList_Seq_Dia.jpg)

The following sequence diagram shows how a save operation goes through the Storage component:
![Sequence Diagram for Save operation](./images/Save_Seq-Dia.jpg)
Copy link

Choose a reason for hiding this comment

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

Perhaps you could standardize the software used to generate the sequence diagrams rather than having only some hand-drawn


#### Sequence Diagram for "Add Meal" Command

![Add Meal Sequence Diagram](images/AddMeal_Sequence_diagram.png)
Copy link

Choose a reason for hiding this comment

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

It is unclear what the hexadecimal values in the sequence diagram represent


**Step 4**: The newly added `Meal` object is returned to the `AddMealCommand` to display as part of the `CommandResult`.

![Add Meal Step 4](images/AddMeal_Step_four_diagram.png)
Copy link

Choose a reason for hiding this comment

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

image

These diagrams are confusing and hard to understand


The overall design that enables this functionality is described generically by the following sequence diagram.

![Edit Command generic sequence](images/editCommand.png)
Copy link

Choose a reason for hiding this comment

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

image
Arrows representing method returns should be dashed arrows.

Copy link

@Metanyu Metanyu left a comment

Choose a reason for hiding this comment

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

Very detailed guide overall, maybe some changes are needed so that the guide looks more professional and unified. Great work!

## Design & implementation
### DailyRecord component
API: `DailyRecord.java`
![Diagram for DailyRecord Component](./images/DailyRecord_API_UML.jpg)
Copy link

Choose a reason for hiding this comment

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

Great work! Maybe can use consistent formatting for all diagrams so that readability can be improved and the DG can look more unified overall.

day, and removing items such as meals or water entries. Each modification is logged for traceability.
- **Calculates key daily statistics:** `DailyRecord` is capable of calculating the total calories burned from the recorded `Day` and the
calories gained from the `MealList`. It can also sum the total water intake for the day.
- **Provides a comprehensive summary:** The class’s `toString()` method generates a detailed summary of the day’s activities, including
Copy link

Choose a reason for hiding this comment

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

I like the clear description of functionality for this component!


### Sequence Diagram

![Sequence Diagram for WeeklySummary feature](./images/History%20WeeklySummary%20UML%20Sequence%20Diagram.png)
Copy link

Choose a reason for hiding this comment

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

Very easy to understand description and sequence diagram! Maybe more consistent formatting and more contrast coloring (the black activation bar against black background is hard to see)


The overall design that enables this functionality is described generically by the following sequence diagram.

![Edit Command generic sequence](images/editCommand.png)
Copy link

Choose a reason for hiding this comment

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

Potential diagram bug: Is the activation bar for Model class too long ?


The overall design that enables this functionality is described generically by the following sequence diagram.

![Edit Command generic sequence](images/editCommand.png)
Copy link

Choose a reason for hiding this comment

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

Another potential bug: activation bar for Parser never ended?

This flow allows users to easily create structured workout routines, customizing their fitness journey directly within BuffBuddy.

The overall design that enables this functionality is described generically by the following sequence diagram.
![](images/createCommand.jpeg)
Copy link

Choose a reason for hiding this comment

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

Maybe can use digital tools for this diagram because it's hard to clearly make out the details in this.

nirala-ts and others added 30 commits November 12, 2024 13:06
Update ProgCommandFactory to ProgrammeCommandFactory
Edited programme exceptions
made changes to exceptions
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.