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-T10-1] FridgeFriend #22

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

Conversation

kwokyto
Copy link

@kwokyto kwokyto commented Mar 4, 2021

FridgeFriend is an app for people who cook managing food in the fridge, optimised for use via a Command Line Interface (CLI). If you can type fast, FridgeFriend can track your cold or frozen groceries faster and easier than any other apps.

nagiteja added a commit to nagiteja/tp that referenced this pull request Mar 15, 2021
@kwokyto kwokyto closed this Mar 17, 2021
@okkhoy okkhoy reopened this Mar 30, 2021
@Tyuanyuan
Copy link

Is it better to show the relations with other classes in the diagram?
image

@Tyuanyuan
Copy link

Is it better to save the categories as enum?
image

@Tyuanyuan
Copy link

Should the program be started by users?
image

Copy link

@boonjuey boonjuey left a comment

Choose a reason for hiding this comment

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

image

Would it be better to describe the fridge class when talking about earlier components (as there was no references to it prior to the command component diagram)?

@boonjuey
Copy link

boonjuey commented Apr 1, 2021

image
image

From the features it seems there aren't specific methods to handle various subclasses for food. Since enum is already used to indicate food categories, maybe the subclasses could be done away with?

@Tyuanyuan
Copy link

"use" doesn't sound like a composition relationship?
image

@boonjuey
Copy link

boonjuey commented Apr 1, 2021

image

For this self-invocation, I think there's a missing arrow pointing back to the bar?
(As highlighted here:)
image

Though, there is no loss in information by omitting this, it will be abit inconsistent with the other diagrams.

Copy link

@JoviYeung92 JoviYeung92 left a comment

Choose a reason for hiding this comment

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

Overall great job guys !

@@ -1,34 +1,726 @@
# Developer Guide

## Design & implementation
## Introduction

Choose a reason for hiding this comment

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

Hi, introduction should be below the table to contents right?

{Describe the value proposition: what problem does it solve?}
### Architecture

![Architecture Diagram](diagrams/diagram_images/ArchitectureDiagram.png)

Choose a reason for hiding this comment

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

DG review2

Is the circled part your data? It might better to group them into data, logic, etc....


Given below is the sequence diagram for the interactions within the main application logic.

![MainLogicSequenceDiagram](diagrams/diagram_images/MainLogicSequenceDiagram.png)

Choose a reason for hiding this comment

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

DG review3
The circled part looks like they are joined together maybe try to leave a gap in between.


Given below is the sequence diagram for the AddCommand workflow.

![AddCommandSequenceDiagram](diagrams/diagram_images/AddSequenceDiagram.png)

Choose a reason for hiding this comment

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

DG review4

What does it returns over here the highlighted part?

@boonjuey
Copy link

boonjuey commented Apr 1, 2021

image

Would getCommand() be better classified as a UI method?

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.

8 participants