public double CalculatePrice(int numberOfToppings, PriceDefinition priceDefinition) { double price = priceDefinition.BasePrice; price = numberOfToppings * priceDefinition.PricePerUnit + price; price = _taxCalculator.GetTotalAfterTax(price); return(price); }
// How many arguments is too many? // Is this function doing too much? (Think single responsibility) // Do we have to pass all these arguments every time? // Can naming of the arguments be more clear? public double CalculatePrice() { double price = priceDefinition.BasePrice; price = priceDefinition.PricePerUnit * this.numberOfToppings + price; // Is there repeated code? (Follow Don't Repeat Yourself or DRY principle) // Is this code closed to modifications and open to new province and tax rules extensions? (Open closed principle) // Is there a way to break down this piece of code into even smaller pieces of functionality (provincial and federal tax)? price = _taxCalculator.GetTotalAfterTax(price); // if (province == "AB") // price = price + price * 0.05; // else if (province == "ON") // price = price + price * (0.05 + 0.08); return(price); }