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

code review #2

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

code review #2

wants to merge 2 commits into from

Conversation

robertdhart99
Copy link
Collaborator

The project was ontime and all the tests passed. This has everything that is needed from the course resoruces. It took me a little bit to udnerstand the project but once I did i had no problems.

  1. I learned to input exception handling.
    void createFood_validateReturnFoodWithFoodNameFoodCategoryAndFoodPrice() throws Exception
    I had not implemented any and forgot about it when working in sprint 1.

  2. I learned about Hashmaps and how they can be used to organize objects and put them in a dynamic list that can be updated, deleted, saved, etc. This is just a snippet of code showing it being used in the shoppingCart class.
    public ShoppingCart() {
    orders = new HashMap<String, CartOrder>();
    }

    public ShoppingCart(String shoppingId) {
    this.shoppingId = shoppingId;
    orders = new HashMap<String, CartOrder>();
    }

    public void addOrUpdateOrderToCart(CartOrder order) {
    String cartOrderFoodId=order.getFood().getFoodId();
    if(orders.containsKey(cartOrderFoodId)){
    CartOrder currentOrder=orders.get(cartOrderFoodId);
    currentOrder.setFoodQuantity(currentOrder.getFoodQuantity()+order.getFoodQuantity());
    orders.put(cartOrderFoodId,currentOrder);
    }
    else{
    orders.put(cartOrderFoodId, order);
    }
    }

  3. I had not used the verify function before and will be using it in the future. I have mostly been using assertEquals
    verify(foodDao, atLeastOnce()).createFoodItem(food);

overall:
I added a test to search by category following TDD guidlines.
I added a getFoodByCategory to the stub
I added two methods in shopping cart to calculate the tax and the total cost with tax that can each be called independetly so tax can be showed by itself in details. Food is not usually taxed but fast food is and the base is 5.75%

Comment on lines +64 to +71
public double totalCostWithTax(){
double totalWithTax = taxCalculator() +totalEstimatedCostOfEntireCart();
return totalWithTax;
}
public double taxCalculator(){
double tax = totalEstimatedCostOfEntireCart() * .0575;
return tax;
}
Copy link

@discospiff discospiff Oct 12, 2021

Choose a reason for hiding this comment

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

This appears to be an incomplete feature. Avoid adding new, incomplete functionality .

  1. Only features that meet the Definition of Done should be merged to Master.
  2. Unused code is clutter, which increases technical debt.

Comment on lines +49 to +55
@Override
public Food getFoodByCategory(String category) {
Food food = new Food();
food.setFoodCategory("Snacks");
food.setFoodName("Potato Chips");
return food;
}

Choose a reason for hiding this comment

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

This appears to be an incomplete feature. Avoid adding new, incomplete functionality .

  1. Only features that meet the Definition of Done should be merged to Master.
  2. Unused code is clutter, which increases technical debt.

@discospiff
Copy link

This branch introduces new functionality, which is not complete. New features should be determined by the Product Owner of the team, as part of the scrum process. My recommendation:

  1. Don't merge this branch, as incomplete features should not be in main.
  2. If the team sees values in these features, keep this branch open, and complete the features, then merge to main once complete.

RIA03B added a commit that referenced this pull request Oct 24, 2021
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.

None yet

2 participants