public class ShoppingCart {
private final List<Item> items;
public ShoppingCart() {
items = new ArrayList<Item>();
}
public void addItem(Item item) {
items.add(item);
}
public double calcTotalCost() {
double total = 0.0;
for (Item item : items) {
total += item.getPrice();
}
return total;
}
}
...and this is the test case:
@Test
public void calculateTotalCost() {
ShoppingCart instance = new ShoppingCart();
Item a = new Item("gloves", 23.43);
instance.addItem(a);
Item b = new Item("hat", 10.99);
instance.addItem(b);
Item c = new Item("scarf", 5.99);
instance.addItem(c);
double totalCost = instance.calcTotalCost();
assertEquals(40.41, totalCost, 0.0001);
}
I have to ask though, does TDA it all come down to a matter language and semantics. Consider the example above, is the client telling or asking? Although this code is much better than my ask don't tell example, I think that a case can be made for saying that the client is asking for the total price. Consider the following issues:
- In telling the Shopping Cart to return total price how do you know that you’re not querying the internal state of the object? Looking at the ShoppingCart code, you can see that the total cost is not part of the object’s direct internal state1, but the object calculates it and hence the total price is part of the object’s derived internal state and this is being returned to the caller.
- In a TDA world, why would the client need the total cost? To figure out if you need to add a shipping cost? That can be done by the ShoppingCart. To submit the bill to the appropriate payment system? Again that can be done by the shopping cart.
If you agree that return values reflect the internal state of an object, whether direct or inferred then, as Mr Spock would say, logic dictates that you’d have to conclude that all method signatures would have a void return value, never throw exceptions and handle all errors themselves and look something like this:
public void processOrder(String arg1, int arg2);
And this is where some of the logic can start to unravel. Doesn’t a ShoppingCart contacting the Visa or Mastercard payment system break the single responsibility principle (SRP)? The WareHouse also needs to be informed that the order can be shipped. Is that the responsibility of the ShoppingCart? And what if we wanted to print an itemised bill in PDF format? You can tell the ShoppingCart to print itself, but are we breaking the SRP by adding printing code to the ShoppingCart however small it may be? Perhaps this requires further thought, so take a look at the Communication Diagram below:
This diagram shows the straight forward scenario of adding an item to the ShoppingCart. TDA works perfectly here because there are no branches and no decisions, the whole process is linear: the browser TELLS the OrderController to add an item and the OrderController TELLS the ShoppingCart to add an item to itself.
Now, take a look at the next scenario. Here the user is confirming the order and then sitting down to eagerly await its delivery.
If you look at the diagram, you’ll see that the browser TELLS the OrderController to confirm the order. The OrderController TELLS the ShoppingCart to confirm the order. The ShoppingCart TELLS the Visa system to charge the total to the user’s card. If the the payment goes through okay then the Visa card TELLS the WareHouse to ship the order.
Now this works as a design if you accept that a ShoppingCart is responsible for maintaining a list of items the user wants and paying for those items. You’ve also got to accept that the Visa card is responsible for shipping them to the customer: all of which doesn’t make sense to me as it breaks the SRP and, in my opinion, the SRP is one of the fundamental features of good software design. Besides, when I’m doing my shopping in the supermarket I don’t ask my shopping cart to pay the bill, I have to get my wallet out. If you take that analogy further, then you’ll conclude that it’s some other object's responsibility to marshall the flow of the transaction; the OrderController’s perhaps?
In this final diagram you can see that the browser TELLS the OrderController to confirm the order. The OrderController ASKS the ShoppingCart for the total cost and then TELLS the Visa object to pay the bill and finally if the Visa returns success the it TELLS the WareHouse to ship the ShoppingCart. To me this design makes more sense, it’s tell don’t ask with a touch of pragmatism.
Now don’t get me wrong, I like the idea of tell don’t ask it makes perfect sense, but it’s not perfect and it can stumble. If you search the Internet for examples you often find that they’re linear in nature, reflecting the first two diagrams above where A calls B, B calls C and C calls D etc. This doesn’t reflect most applications as at some point in your programs execution you’re forced to ask an object for data, and to make a decision based upon that data. The second reason tell don’t ask stumbles is that it’s so easy to fall into the trap of asking an object for data without even thinking about it. For example, take a look at the snippet below:
AnnotationChecker checker = new AnnotationChecker(clazz);
if (checker.isMyComponent()) {
Object obj = clazz.newInstance();
result.put(clazz, obj);
}
This example, plucked from my github dependency-injection-factory sample project shows me asking an object its state and using that information to make a decision. Procedural programming strikes again...
1Direct internal state: a value held in an instance variable of an object. Derived or inferred internal state: a value returned by an object that is calculated or derived from the instance variables of that object.
6 comments:
I think that you have missed the point a little. Indeed the cart should not be responsible for payment because it breaks SRP, but you can design it to still use the TDA and have single responsiblity. Enter strategy pattern:
class Cart {
public boolean checkout(CheckoutStrategy strategy){
return strategy.checkout(this.calculateCosts(),this.items);
}
}
class CheckoutStrategy {
public CheckoutStrategy(CreditCardAdapter ca, WarehouseService wa);
public boolean checkout(List<> items, Money cost){
if(ca.pay(cost)){
return wa.process(items);
}
return false;
}
:)
Nice post Roger. I can't totally agree more.
Cheers,
Bruno
Wojciech
Thanks for the comment. I thought that someone may mention this technique and wondered about mentioning it. It's certainly a technique I'd use, although I remain unconvinced about whether or not adding a method like:
public boolean checkout(CheckoutStrategy strategy)
to a ShoppingCart object breaks the SRP. Although the Cart doesn't contain any checkout code as that's held in the CheckoutStrategy, it still has a checkout method denoting that it has checkout responsibility. Hmmm.
This is alone a topic for a huge post, but in my opinion it works as the following:
Objects exist in different contexts, the context are defined in code as interfaces, a single interface defines methods that are compliant to SRP.
Effectivly we have:
class Cart {
//item list and other fields
}
class CartInCheckoutContext extends Cart implements CheckoutContext {
public CartInCheckoutContext(Cart cart);
public boolean checkout(CheckoutStrategy strategy);
}
in this way we don't break SRP and we comply to TDA. I know that this code looks some kind of ugly, but it is because it's written in Java. In Ruby or Scala, where we have mixins it would be much better.
The TDA technique is in my opinion only a part of a broader paradigm called Data Context Interaction and the above code is an illustration of it.
Exactly, im my opinion having a method that just delegates also breaks SRP.
The TDA principle says that you should not make any decisions using the object state. In your example the shopping cart price is not used to make any decisions. So it can be passed into the visa.PayBill() method. And this does not violate the TDA. However, the price also can be used to make a decision (e.g. we don't process any orders that are more than 1000 USD). I've written the following example that shows how to satisfy the TDA principle in this case. It's a bit verbose, but it is also very flexible.
class OrderProcessor
{
private IShoppingCartWithPriceApproval _shoppingCart;
private IVisa _visa;
private IWarehouse _warehouse;
public OrderProcessor(IShoppingCartWithPriceApproval shoppingCart, IVisa visa, IWarehouse warehouse)
{
_shoppingCart = shoppingCart;
_shoppingCart.PriceApproved += new PriceApprovedEventHandler(PayBill)
_visa = visa;
_visa.PaymentCompleted += new PaymentCompletedEventHandler(ShipOrder)
_warehouse = warehouse;
}
public void ProcessOrder()
{
_shoppingCart.CheckThePrice();
}
public void PayBill(double price)
{
_visa.PayBill(price);
}
public void ShipOrder()
{
_warehouse.ShipOrder();
}
}
public delegate void PriceApprovedEventHandler(double price);
class ShoppingCartThatApprovesThePrice : ShoppingCart, IShoppingCartWithPriceApproval
{
public event PriceApprovedEventHandler PriceApproved;
public void CheckThePrice()
{
if (Price <= 1000)
if (PriceApproved != null)
PriceApproved(Price);
}
}
Post a comment