[CS2113-T17-2] CafeCTRL #14
Conversation
Added Selecting Storage
There was a problem hiding this comment.
Overall, the team did a great job in their implementation.
In terms of software engineering practices, we can tell that most code quality and coding standards were abided to (e.g. naming convention, less than 80 characters per line). However, one suggestion that I would make is to remove unused / dead code or leave a comment there to tell the reader the purpose of not deleting it entirely.
In terms of Object Oriented Programming, I really enjoy seeing that there is a separation of concerns and that there are several domains created for different purposes. For instance, command is command, menu is menu and pantry is pantry. This is from personal experience but perhaps we can abstract the logic even more.
Note: This is for educational purposes. Do not attempt this unless you have ample time and find what I say make sense and no additional marks would be given
- Maybe we can read up on Domain Driven Development (DDD). We can declare the domains that is needed first (maybe in terms of interfaces) before we start working on it.
- For instance, let us consider the Chef object. Does a normal chef have a UI object with him? What if we shift into cookDish method instead and use it as a dependency rather than a member? The issue with this is it violates the Single Responsibility Principle, we try to combine the printing logic with the other logic.
- Perhaps we can consider implementing an interface called ChefInterface that has the necessary attributes that belong to a Chef and implement it, and maybe create an overarching service class (or use the command class) to make the classes interact with each other like how normal chefs would.
| ui.printLine(); | ||
| } | ||
| } while (!command.isExit()); | ||
| //this.storage.saveAll(this.menu, this.orderList, this.pantry); |
There was a problem hiding this comment.
Why don't we remove this dead code since it is not being used here?
| } | ||
|
|
||
| public static void main(String[] args) throws IOException { | ||
| CafeCtrl cafeCtrl = new CafeCtrl(); |
There was a problem hiding this comment.
No change is required here, I was just curious if there is a need for this line here. Why not create a method called initCafe() rather than creating a new instance of it?
| * Adds a menu item to the user | ||
| */ | ||
| public class AddDishCommand extends Command { | ||
| public static final String COMMAND_WORD = "add"; |
| public static final String COMMAND_WORD = "view_stock"; | ||
| protected Ui ui; | ||
| protected Pantry pantry; | ||
|
|
There was a problem hiding this comment.
Perhaps we can review all the files and ensure consistency in the spacing between each line?
| //@@author DextheChik3n | ||
| public static ArrayList<String> encodeMenu(Menu menu) { | ||
| /* | ||
| ArrayList<Dish> dishArrayList = menu.getMenuItemsList(); | ||
| String tasksFilePathString = this.fileManager.openTextFile(); | ||
|
|
||
| if (dishArrayList.isEmpty()) { | ||
| this.fileManager.overwriteFile(tasksFilePathString, ""); //overwrite text file to store empty text | ||
| } | ||
| //todo: remove testing code | ||
| dishArrayList.add(new Dish("test", (float) 1.2)); | ||
| dishArrayList.add(new Dish("test", (float) 1.2)); | ||
|
|
||
| //input arraylist data into text file | ||
| for (int i = 0; i < dishArrayList.size(); i++) { | ||
| String taskDataRow = "chicken rice | 5.00 | rice 50g"; | ||
|
|
||
| if (i == 0) { | ||
| this.fileManager.overwriteFile(tasksFilePathString, taskDataRow); | ||
| } else { | ||
| this.fileManager.appendToFile(tasksFilePathString, taskDataRow); | ||
| } | ||
| } | ||
| */ | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Maybe we can remove this dead code since it returns null and does not do anything
There was a problem hiding this comment.
we are still in progress in implementing the storage to text file functionality so it was left commented out
| // ArrayList<String> encodedMenu = this.fileManager.readTextFile(FilePath.MENU_FILE_PATH); | ||
| // return Decoder.decodeMenuData(encodedMenu); |
There was a problem hiding this comment.
Maybe we can return the dead code here too
|
One thing to take note of: Since you are dealing with money, there is a possibility that other teams might break your product using big numbers (causing overflow), negative numbers, non-2 decimal places pricing. Extra caution had to be taken care of for this (A.K.A. consider unit testing + assertions before mock PE 1) |
There was a problem hiding this comment.
arrow 22 has a typo ? maybe you wanted to specify it as showMenuBottom(varname: String) ?
There was a problem hiding this comment.
Maybe modify arrow to show showToUser(<static var name>, ..)
There was a problem hiding this comment.
although its returning void, you may want to consider adding the dotted arrows here to standardize with/follow the other diagrams
docs/DeveloperGuide.md
Outdated
| @@ -4,10 +4,154 @@ | |||
|
|
|||
| {list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} | |||
There was a problem hiding this comment.
Maybe remove the default placeholder text
docs/DeveloperGuide.md
Outdated
|
|
||
|  |
There was a problem hiding this comment.
The accessibility of a variable or method should be + (public) or - (private) instead of colours by default in plantUML
| ### Storage | ||
| API: [Storage.java]({repoURL}src/main/java/seedu/cafectrl/storage/Storage.java) | ||
|
|
||
|  |
There was a problem hiding this comment.
Same issue regarding accessibility as before
|
|
||
| ### Edit Price | ||
|
|
||
|  |
There was a problem hiding this comment.
Activation bar for 6th step should start after the arrow
|
|
||
| ### Add Dish | ||
|
|
||
|  |
docs/DeveloperGuide.md
Outdated
|
|
||
|  | ||
|
|
||
| Figure 1: Execution of list_menu command |
docs/DeveloperGuide.md
Outdated
|
|
||
| Note that `CafeCtrl` only have access to the interface `ParserUtil` although the run-time type object is `Parser`. With this, we are able to decrease coupling between `CafeCtrl` and `Parser`, allowing for easier maintenance. This also ensures the testability as we could provide mock or stub dependencies during testing, we could isolate the behavior of the class and focus on unit testing without external dependencies. | ||
|
|
||
|  |
There was a problem hiding this comment.
Alt should have multiple braches, if there's only one possible branch, should use opt instead
| ### Storage | ||
| API: [Storage.java]({repoURL}src/main/java/seedu/cafectrl/storage/Storage.java) | ||
|
|
||
|  |
There was a problem hiding this comment.
The storage class diagram does not follow CS2113 standard. You may want to look at the notes. For example, shouldn't have the C logo etc.
|  | ||
|
|
||
|  | ||
|
|
There was a problem hiding this comment.
Adding the number for each step is not necessary
should these arrow be separated from each other?
|
|
I really like the way you include both class diagram and sequence diagram to illustrate your components and features! |
|
|
||
| ### Edit Price | ||
|
|
||
|  |
There was a problem hiding this comment.
It seems like the method called in 6th step doesn't follow the sequence diagram format?
|
|
||
| ### Edit Price | ||
|
|
||
|  |
There was a problem hiding this comment.
8th step arrow may need to point to the start of the activation bar
docs/DeveloperGuide.md
Outdated
| - `Parser` : Makes sense of user input to return the appropriate command | ||
| - `Command` : Executes the comman requested by the user. | ||
|
|
||
| How the architecture components interact with each other |
There was a problem hiding this comment.
Should this be a heading?
Looking at it on the website, it seems like normal text; but I think it signifies a new segment. See this:
There was a problem hiding this comment.
Thanks for pointing out! I think your suggestion makes sense and I will edit accordingly!
docs/DeveloperGuide.md
Outdated
|
|
||
|  | ||
|
|
||
| *Figure 4: Parser Class Diagram* |
There was a problem hiding this comment.
Can you make the diagram higher resolution?
It is hard to read on the website, see
docs/DeveloperGuide.md
Outdated
| - is composed of `FileManager` object as the text file needs to be located first before reading or writing. | ||
|
|
||
| ### Data Component | ||
| API: [] |
There was a problem hiding this comment.
What is this supposed to mean?
It confuses me and I think you should clarify this
There was a problem hiding this comment.
We forgot to add hyperlink here! It is updated in the latest version
docs/DeveloperGuide.md
Outdated
| ### Add Dish | ||
|
|
||
|  | ||
| *Figure 8: Execution of add_dish command* |
There was a problem hiding this comment.
The "Figure 8" text is in a weird location besides the diagram
There was a problem hiding this comment.
oops I forgot to add a newline thanks for pointing it out 👍
docs/DeveloperGuide.md
Outdated
| e.g. (1. Chicken Rice $2.50) is shown. | ||
|
|
||
| ### Add Order | ||
| A add_order command can be used to add `order` to an `orderList` in `Sales`. |
There was a problem hiding this comment.
You should write "an add_order" instead of "a add_order"
docs/DeveloperGuide.md
Outdated
| ## Product scope | ||
| ### Target user profile | ||
|
|
||
| Café proprietors seeking for a software solution to optimize the management of their café's operations |
There was a problem hiding this comment.
Optimize it how? Could you explain a bit more?
|
|
||
| The Sequence Diagram below shows how the components interact with each other for the scenario where the user issues the command `delete 1`. | ||
|
|
||
|  |
There was a problem hiding this comment.
Hmmm I am not sure which part you are referring to, could you specific which statement is it?
|
|
||
| API: [ListMenuCommand.java]({repoURL}src/main/java/seedu/cafectrl/command/ListMenuCommand.java) | ||
|
|
||
| When the `execute()` method of ListMenuCommand is invoked in Main, it checks if the size of the menu by running `menu.getSize()`. |
There was a problem hiding this comment.
It's nice to check whether the list is empty or not before the execution.
|
|
||
| * *glossary item* - Definition | ||
| ### User stories |
There was a problem hiding this comment.
The user stories are clear and considerate enough to make sure all kinds of user can realize their needs.
|
|
||
| ### Edit Price | ||
|
|
||
|  |
There was a problem hiding this comment.
in 6th step should have an arrow before
There was a problem hiding this comment.
hmm I dont quite get what you mean, could you specify where should the arrow be at?
…es-v2.1 Task 320, Error handling for decoding of sales.txt, list_ingredients and list_sale command
# Conflicts: # src/main/java/seedu/cafectrl/ui/ErrorMessages.java # src/main/java/seedu/cafectrl/ui/Messages.java
Include patter and mathcer in PPP
list_sale command.
Pantry class in DG.
Edit UG v2.1
Update DG, UG and PPP
Update HelpCommand
Update PPP
Edit PPP to reduce page length
Add manual testing for edit price
Edit PPP to reduce page length
# Conflicts: # docs/team/ziyi105.md
Reduce PPP length
Disable assertion
CaféCTRL aims to optimize managing of inventory and cash flow in a restaurant. Our CLI platform empowers users to streamline stock inventory, menu and orders. Users will also briefly be able to gain valuable insights through comprehensive sales reporting, enabling them to analyze sales trends and calculate revenue/profit margins, eliminating the need for cross-platform management.