Bad idiom: assign and return

Here is the first example for an ill-designed method. I encountered this idiom as I was solving a bug (gross estimate: 60 minutes), in a project based on the Eclipse Java compiler (JDT).

The problem is illustrated by the computeTotal(Pricer p) method below (in Eclipse JDT it occurs in the Expression.resolveType(BlockScope s) method). Can you spot the bug?

class Item { ... }
class Pricer { public double priceOf(Item i) { ... } }

abstract class Cart {

   Cart(List<Item> is) { items = is; }
   double computeTotal(Pricer p) {
      double result = 0.0;
      for(Item i : items)
         result += p.priceOf(i);
      return result;
   }

   protected final List<Item> items;
   public double total;
}

class DiscountCart extends Cart {

   DiscountCart(List<Item> is) { super(is); }
   void add(Item i) { is.add(i); }

   public double computeTotal(Pricer p) {
      return super.computeTotal(p)*0.85;
   }
}

computeTotal() must return a value. so you typically write code the computes total price and returns it. There is nothing that reminds you to store this result at the total field (inherited from the superclass).
Even in this small snippet it is likely to miss this subtle issue, nonetheless if Cart has many more methods, and computeTotal() is just one of serveral methods which you override/implement.

The underlying problem here is that of overlapping conerncs: We expect a single method to act both as a getter and as a mutator. The developer of subclasses is likely to overlook one of these concerns.

It seems that the best solution is to split the responsibilites: Change the return type of computeTotal() to void, thereby indicating that it is a mutator (of the total field) rather than a getter. This will make the method more coherent, thus clarifying its contract.

Note that the problem is not due to Cart.total being a public field. Even if we had a getter method that encapsulates Cart.total, such a method could not have calculated the total value without a Pricer object. So you'd have to add a Pricer parameter to your getter method, which then becomes just a renamed version of computeTotal().