Over a million developers have joined DZone.

Connascence of Algorithm

· DevOps Zone

Discover how to optimize your DevOps workflows with our cloud-based automated testing infrastructure, brought to you in partnership with Sauce Labs

This is the fourth post in a series in which I am test-driving a classic code kata, and using only the rules of connascence to tell me what to refactor.

If you have been following along, you’ll recall that my most recent refactor was to create a new domain object for MultiBuyDiscount, thereby alleviating a nasty case of Connascence of Position. The tests pass, so let’s review what connascence remains in this code:

  • Level 1, Connascence of Name, for example because various classes know the names of methods to call on each other. This is benign, and isn’t going away anytime soon.
  • Level 2, Connascence of Type, for example because the test knows which classes to instantiate. Similarly, this is largely benign in this code.
  • Level 4, Connascence of Algorithm. There are two problems here: Firstly, knowledge of the parameters of scan() persists across calls. This means that the caller (in this case, the test) has to know something about how the Checkout works, in order to get it to behave predictably. And secondly, the Checkout itself knows that the MultiBuyDiscount has a trigger, an SKU and a discount amount. The MultiBuyDiscount itself has no privacy, and if the design of that domain concept changes we would have to change code in the Checkout too.

coa

I plan to fix these two instances of Connascence of Algorithm in the order described above. First, the coupling between the test and the Checkout.

The state of the Checkout instance now depends on the value of the parameters to the various calls to scan(). If I change the special offers while scanning the items in a single basket, the result is inconsistent behaviour. For example:

@Test
public void connascenceOfExecutionOrder() {
  Money priceOfA = randomPrice();
  Money firstTotal = new Checkout()
      .scan("A", priceOfA, new MultibuyDiscount("A", Money.fromPence(20), 1))
      .scan("A", priceOfA, new MultibuyDiscount("A", Money.fromPence(20), 2))
      .currentBalance();
  Money secondTotal = new Checkout()
      .scan("A", priceOfA, new MultibuyDiscount("A", Money.fromPence(20), 2))
      .scan("A", priceOfA, new MultibuyDiscount("A", Money.fromPence(20), 1))
      .currentBalance();
  assertEquals(firstTotal, secondTotal);
}

This test fails, which is surely a defect in the design. Thankfully the remedy is simple: I pass the special offer to the Checkout via its constructor, not via the scan() method:

public class Checkout {
  //...
  private MultibuyDiscount discount;
 
  public Checkout(MultibuyDiscount discount) {
    this.discount = discount;
  }
 
  public Checkout scan(String sku, Money price) {
    balance = balance.add(price);
    if (sku.equals(discount.discountSku))
      discountItems++;
    if (discountItems == discount.discountTrigger)
      balance = balance.subtract(discount.discount);
    return this;
  }
}

This fixes the worst of the Connascence of Algorithm, which coupled the test to the state of the Checkout instance. Next I tackle the other instance of Connascence of Algorithm, between the Checkout and the MultiBuyDiscount. This is a case of “Feature Envy”, and I must admit that I think twice before fixing it at all. It could be argued that the code is reasonably okay as it stands, and that this coupling would only need to be fixed if  the business introduced a different kind of discount in the future. However, the code as it stands is aesthetically unpleasant, so I decide to press on.

I move the guts of the discount algorithm out of the Checkout:

public class Checkout {
  //...
  public Checkout scan(String sku, Money price) {
    balance = balance.add(price);
    balance = balance.subtract(discount.discountFor(sku));
    return this;
  }
}

and into the MultiBuyDiscount:

public class MultibuyDiscount {
  private String watchedSku;
  private Money discount;
  private int trigger;
  private int itemsSeen = 0;
 
  public MultibuyDiscount(String sku, Money discount, int trigger) {
    this.watchedSku = sku;
    this.discount = discount;
    this.trigger = trigger;
  }
 
  public Money discountFor(String sku) {
    if (sku.equals(watchedSku))
      itemsSeen++;
    return (itemsSeen == trigger) ? discount : Money.ZERO;
  }
}

So, that’s it — the Connascence of Algorithm is all gone. Well, not quite: In making this change I have only moved the connascence around! Suppose I create two Checkouts, but have them share a MultibuyDiscount? The following test fails:

@Test
public void independentCheckouts() {
  Money priceOfA = randomPrice();
  MultibuyDiscount discount = new MultibuyDiscount("A", Money.fromPence(20), 2);
  Checkout checkout1 = new Checkout(discount);
  Checkout checkout2 = new Checkout(discount);
  checkout1.scan("A", priceOfA);
  checkout2.scan("A", priceOfA);
  assertEquals(priceOfA, checkout1.currentBalance());
  assertEquals(priceOfA, checkout2.currentBalance());
}

The second customer gets a discount, despite only buying one “A”!

The discount object represents the state of the customer’s basket, because it remembers how many items of the trigger SKU have been scanned. So the client (again, only a test in our case, but it could be anything) must remember to create new discount objects each time it creates a new checkout. That’s Connascence of Algorithm. The question is: how to fix it?

  1. I could ensure that each Checkout instance creates its own discount rule, so that they are never shared. I would have to be careful not to re-introduce the Connascence of Value, so it would still need to be the test that would know the rule’s details.
  2. I could look for a way to split the MultiBuyDiscount into two pieces: one to know the business rules, and the other to keep a track of how many trigger SKUs have been scanned in the current customer’s basket. The Checkout could hold an instance of the latter, while the test holds the former.

When I look at these in detail I see that they are essentially the same solution. And since I’m working in Java, my options are fairly closely constrained. The rule object passed from the test into the Checkout’s constructor must be a factory that the Checkout can use to create its own personal discounter. I create a new factory class:

public class MultibuyDiscountFactory {
  private String watchedSku;
  private Money discount;
  private int trigger;
 
  public MultibuyDiscountFactory(String sku, Money discount, int trigger) {
    this.watchedSku = sku;
    this.discount = discount;
    this.trigger = trigger;
  }
 
  public MultibuyDiscount create() {
    return new MultibuyDiscount(watchedSku, discount, trigger);
  }
}

and then use it in the Checkout:

public class Checkout {
  //...
  public Checkout(MultibuyDiscountFactory discount) {
    this.discount = discount.create();
  }
}

Well, that was a lot more work than I expected when I first noticed the Connascence of Algorithm (aka Feature Envy) back in the previous post! Next time I will revisit the decision to use a factory here…


Download “The DevOps Journey - From Waterfall to Continuous Delivery” to learn learn about the importance of integrating automated testing into the DevOps workflow, brought to you in partnership with Sauce Labs.

Topics:

Published at DZone with permission of Kevin Rutherford, DZone MVB. See the original article here.

Opinions expressed by DZone contributors are their own.

The best of DZone straight to your inbox.

SEE AN EXAMPLE
Please provide a valid email address.

Thanks for subscribing!

Awesome! Check your inbox to verify your email so you can start receiving the latest in tech news and resources.
Subscribe

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}