-
Notifications
You must be signed in to change notification settings - Fork 233
[CS2113-T13-1] KitchenCTRL #63
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
base: master
Are you sure you want to change the base?
[CS2113-T13-1] KitchenCTRL #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good effort so far, but please improve on your standardisation, as well as clean up your code.
| package logic.commands; | ||
|
|
||
| import model.Ingredient; | ||
| import model.Recipe; | ||
| import java.util.ArrayList; | ||
| import model.catalogue.IngredientCatalogue; | ||
|
|
||
| public class Commands { | ||
|
|
||
| // Add ingredient to inventory | ||
| public static void addIngredient(IngredientCatalogue inventory, String name, int quantity) { | ||
| for (Ingredient ing : inventory.getItems()) { | ||
| if (ing.getIngredientName().equalsIgnoreCase(name)) { | ||
| ing.setQuantity(ing.getQuantity() + quantity); | ||
| return; | ||
| } | ||
| } | ||
| inventory.addItem(new Ingredient(name, quantity)); | ||
| } | ||
|
|
||
| // Edit ingredient quantity | ||
| public void editIngredient(IngredientCatalogue inventory,String name, int newQuantity) { | ||
| for (Ingredient ing : inventory.getItems()) { | ||
| if (ing.getIngredientName().equalsIgnoreCase(name)) { | ||
| ing.setQuantity(newQuantity); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // List all ingredients in inventory | ||
| public void listIngredients(IngredientCatalogue inventory) { | ||
| for (Ingredient ing : inventory.getItems()) { | ||
| System.out.println(ing.getIngredientName() + ": " + ing.getQuantity()); | ||
| } | ||
| } | ||
|
|
||
| // Delete an ingredient from inventory | ||
| public void deleteIngredient(IngredientCatalogue inventory, Ingredient ingredientToDelete) { | ||
| inventory.deleteItem(ingredientToDelete); | ||
| } | ||
|
|
||
| /* | ||
| * Checks if there are enough ingredients in inventory to cook recipe | ||
| * | ||
| * @paramin recipe | ||
| * @paramout arraylist of missing ingredients and quantity | ||
| */ | ||
| public static ArrayList<Ingredient> getMissingIngredients(IngredientCatalogue inventory, Recipe targetRecipe) { | ||
| ArrayList<Ingredient> missingIngredients = new ArrayList<>(); | ||
| ArrayList<Ingredient> inventoryItems = inventory.getItems(); | ||
| ArrayList<Ingredient> targetRecipeIngredients = targetRecipe.getItems(); | ||
|
|
||
| for (Ingredient requiredIngredient : targetRecipeIngredients) { | ||
| if (!inventoryItems.contains(requiredIngredient)) { | ||
| missingIngredients.add(requiredIngredient); | ||
| } else { | ||
| int index = inventoryItems.indexOf(requiredIngredient); | ||
| Ingredient availableIngredient = inventoryItems.get(index); | ||
| if (availableIngredient.getQuantity() < requiredIngredient.getQuantity()) { | ||
| int shortage = requiredIngredient.getQuantity() - availableIngredient.getQuantity(); | ||
| missingIngredients.add(new Ingredient(requiredIngredient.getIngredientName(), shortage)); | ||
| } | ||
| } | ||
| } | ||
| return missingIngredients; | ||
| } | ||
|
|
||
| public static void cookRecipe(IngredientCatalogue inventory, Recipe recipeToCook) { | ||
| ArrayList<Ingredient> missingIngredients = getMissingIngredients(inventory, recipeToCook); | ||
| if (!missingIngredients.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| ArrayList<Ingredient> inventoryItems = inventory.getItems(); | ||
| ArrayList<Ingredient> ingredientsToCook = recipeToCook.getItems(); | ||
|
|
||
| for (Ingredient requiredIngredient : ingredientsToCook) { | ||
| int index = inventoryItems.indexOf(requiredIngredient); | ||
| Ingredient ingredientInInventory = inventoryItems.get(index); | ||
| ingredientInInventory.subtractQuantity(requiredIngredient.getQuantity()); | ||
| } | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this even being used? If not, please remove this.
src/main/java/KitchenCTRL.java
Outdated
| private Ui ui; | ||
|
|
||
| /** | ||
| * Main entry-point for the java.duke.Duke application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this comment
src/main/java/KitchenCTRL.java
Outdated
|
|
||
| private void exit() { | ||
| ui.showGoodbyeMessage(); | ||
| System.exit(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt this is needed, you can just allow the app to close gracefully.
src/main/java/KitchenCTRL.java
Outdated
| public static void carltonTest() { | ||
| IngredientCatalogue inventory = new IngredientCatalogue(); | ||
| inventory.addItem(new Ingredient("White sugar", 50)); | ||
| inventory.addItem(new Ingredient("Egg", 3)); | ||
| inventory.addItem(new Ingredient("Flour", 50)); | ||
|
|
||
| Recipe recipe = new Recipe(); | ||
| recipe.addItem(new Ingredient("White sugar", 20)); | ||
| recipe.addItem(new Ingredient("Egg", 1)); | ||
| recipe.addItem(new Ingredient("Flour", 10)); | ||
|
|
||
|
|
||
| System.out.println(recipe); | ||
| System.out.println("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); | ||
| System.out.println("PRINTING MISSING"); | ||
| Commands.cookRecipe(inventory, recipe); | ||
| inventory.listItems(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could have been refactored as a JUnit test instead.
|
|
||
| public class Parser { | ||
| public Command parseCommand(String userInput) { | ||
| String[] commands = userInput.split(" ", 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it is still readable, maybe change the name from commands to something less ambiguous here.
| return Files.readAllLines(filePath); | ||
| } | ||
| } catch (Exception e) { | ||
| System.err.println("Error loading file: " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider implementing logging for these error cases.
src/main/java/model/Recipe.java
Outdated
| @Override | ||
| public CommandResult addItem(Ingredient ingredient) { | ||
| items.add(ingredient); | ||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can print an empty CommandResult instead of just null?
src/main/java/commands/Command.java
Outdated
| public Command(String args) { | ||
|
|
||
| } | ||
|
|
||
| public Command() { | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a difference between these two constructors?
| public CommandResult execute() { // Ensure this method exists | ||
| return null; | ||
| } | ||
|
|
||
| public CommandResult execute(IngredientCatalogue inventory) { | ||
| throw new UnsupportedOperationException("override me"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the intention is to override this method, perhaps this can be an abstract method.
| assert name != null && !name.trim().isEmpty() : "Ingredient name must not be null or empty"; | ||
| assert quantity > 0 : "Ingredient quantity must be greater than zero"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please implement proper guard clauses instead of just using assertions.
This applies to most of the command classes.
yijiano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good effort, keep up the good work. Also, thanks for reading my previosu review and making the necessary changes.
| String[] keywordList = ingredientName.toLowerCase().split(" "); | ||
|
|
||
| return items.stream() | ||
| .filter(currIngredient -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using other stream operations such as findFirst which would be more clear.
| // Create the calculation message | ||
| String calculations = existingIngredient.getIngredientName() + ": " + | ||
| "Initial quantity = " + initialQuantity + ", " + | ||
| "Subtracted = " + decreaseQuantity + ", " + | ||
| "Remaining = " + existingIngredient.getQuantity(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there is a way to refactor this so that it can be a method/class of its own?
| for (int i = 0; i < similarRecipe.size(); i++) { | ||
| System.out.println("Type '" + (i + 1) + "' to delete: " + similarRecipe.get(i).getRecipeName()); | ||
| } | ||
| System.out.println("Type '-1' to cancel this action."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this check can be abstracted out? Especially since this check has been used several times.
| case INVENTORY, RECIPE -> { | ||
| // Expecting: add <ingredientName> <quantity> | ||
| String[] parts = args.split(" ", 2); | ||
| if (parts.length < 2) { | ||
| throw new IllegalArgumentException("Invalid format! Usage: add <name> <quantity>"); | ||
| } | ||
|
|
||
| String name = parts[0].trim(); | ||
| int quantity; | ||
| try { | ||
| quantity = Integer.parseInt(parts[1].trim()); | ||
| } catch (NumberFormatException e) { | ||
| throw new IllegalArgumentException("Quantity must be a valid integer!"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe abstract this out so that this method is not so complex. Also, try to follow the switch statement as stated in the code style.
| public AddCommand(String name, int quantity) { | ||
| assert name != null && !name.trim().isEmpty() : "Name must not be null or empty"; | ||
|
|
||
| assert screen == RECIPEBOOK || quantity > 0 : "Quantity must be greater than zero"; | ||
|
|
||
| this.name = name; | ||
| this.quantity = quantity; | ||
| } | ||
|
|
||
| public AddCommand(String name) { | ||
| assert name != null && !name.trim().isEmpty() : "Name must not be null or empty"; | ||
|
|
||
| this.name = name; | ||
| this.quantity = 0; // irrelevant for recipebook screen | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use try-catch rather than just assert to catch malformed inputs.
| return items.stream() | ||
| .filter(currRecipe -> { | ||
| String name = getRecipeNameLowercase(currRecipe); | ||
| return Arrays.stream(keywordList).allMatch(name::contains); | ||
| }) | ||
| .collect(Collectors.toCollection(ArrayList::new)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stream operation findFirst might do well here
| if (choice == 0) { | ||
| addRecipe(recipe); | ||
| return new CommandResult(recipe.getRecipeName() + " added to recipe book."); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if block can be merged with the above.
| return new CommandResult(recipe.getRecipeName() + " does not exist in the recipe book."); | ||
| } | ||
|
|
||
| if (similarRecipe.size() == SINGLE_MATCH && isExactMatchFound(similarRecipe.get(FIRST_ITEM_INDEX), recipe)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if blocks can be combined, and if needed abstract out the condition to its own method/boolean variable.
| */ | ||
| public void updateItem(T oldItem, T newItem) { | ||
| int index = items.indexOf(oldItem); | ||
| if (index != -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can re-use your AddItem method?
| for (Ingredient requiredIngredient : recipeIngredients) { | ||
| if (!inventoryItems.contains(requiredIngredient)) { | ||
| missingIngredients.add(requiredIngredient); | ||
| } else { | ||
| int index = inventoryItems.indexOf(requiredIngredient); | ||
| Ingredient availableIngredient = inventoryItems.get(index); | ||
| if (availableIngredient.getQuantity() < requiredIngredient.getQuantity()) { | ||
| int shortage = requiredIngredient.getQuantity() - availableIngredient.getQuantity(); | ||
| missingIngredients.add(new Ingredient(requiredIngredient.getIngredientName(), shortage)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make this method simpler (abstract out the steps/conditions)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class logo is wrong, should be : instead.
docs/DeveloperGuide.md
Outdated
| |v1.0| user |enter commands into the system| interact with it and perform necessary actions efficiently | | ||
| |v1.0| user |save and retrieve data| have accessible and up-to-date information | | ||
| |v2.0| user |find a to-do item by name| locate a to-do without having to go through the entire list | | ||
| | Version | As a ... | I want to ... | So that I can ... | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add a column for priority, overall looks good!
| @@ -11,7 +11,7 @@ KitchenCTRL uses the following tools for development: | |||
| The design and implementation of KitchenCTRL has been broken down into various sections | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
docs/DeveloperGuide.md
Outdated
| The commands package provides a structured way to define and execute operations in the application. By inheriting from the Command class, each command ensures consistency and adheres to the application's design principles. The CookRecipeCommand is a concrete example of how commands are implemented to perform specific tasks. | ||
|
|
||
| ### Model Component | ||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class logo seems to be incorrect.
IgoyAI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the UML Diagram. Overall good already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presence of non-standard icons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presence of non-standard icons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods in IngredientCatalogue and RecipeCatalogue (e.g., addItem, deleteItem) return CommandResult, but CommandResult itself does not appear in the diagram. If CommandResult is a class or interface, consider adding it or showing the relationship.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diagram suggests that a Recipe can contain multiple Ingredient objects, but there is no line or association from Recipe to Ingredient. If your code has something like a List inside Recipe, you should show that in the diagram with an association or composition arrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presence of non-standard icons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presence of non-standard icons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presence of non-standard icons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Parser uses or depends on Command objects, you might want to show that relationship in the diagram (e.g., an arrow from Parser to Command indicating it instantiates or calls execute()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diagram shows two execute() methods with different signatures on the Command class (e.g., one returning CommandResult and another taking different parameters). Make sure these match the actual method signatures in your code.
Add JavaDoc and my PPP
Carlton PPP
Add Junit test cases for InputParser, updated docs and fix UI bug
Ching whee guides
update PPP
removed add function from update, fixed issue#61
fix issue #62
updating add->edit jump functionality to recipeobok
Fix infinite loop problem
Updated guides for exiting and saving
some bug fixing and UX
…to ChingWhee-Guides
modify image in DG
udpate aboutus
Ching whee guides
Update PPP
Update UserGuide.md
…to ChingWhee-Guides
Ching whee guides
No description provided.