Over a million developers have joined DZone.

Connascence of Position

· 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 part three of 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. Last time I continued working on the checkout kata, and I fixed some Connascence of Meaning by introducing a new class to represent the domain concept of Money. Today I want to take that same code a little further to explore what happens when I continue simply responding to the connascence I see.

Here’s the code so far:

public class CheckoutTests {
  @Test
  public void basicPrices() {
    Money priceOfA = randomPrice();
    Checkout checkout = new Checkout();
    Money priceOfB = randomPrice();
    checkout.scan("A", priceOfA).scan("B", priceOfB);
    assertEquals(priceOfA.add(priceOfB), checkout.currentBalance());
  }
 
  private Money randomPrice() {
    int pence = new Random().nextInt(1000);
    return Money.fromPence(pence);
  }
}
public class Checkout {
  private Money balance = Money.ZERO;
 
  public Checkout scan(String sku, Money price) {
    balance = balance.add(price);
    return this;
  }
 
  public Money currentBalance() {
    return balance;
  }
}

It’s time now to add special offers. I add a test that says we get a discount of 20p if our basket contains two “A” items:

@Test
public void discountForTwoAs() {
  Money priceOfA = randomPrice();
  Checkout checkout = new Checkout();
  checkout.scan("A", priceOfA).scan("A", priceOfA);
  assertEquals(priceOfA.add(priceOfA).subtract(Money.fromPence(20)), checkout.currentBalance());
}

I make this pass naively, by checking the current scanned item against its predecessor:

public class Checkout {
  private String previousSku;
 
  //...
 
  public Checkout scan(String sku, Money price) {
    balance = balance.add(price);
    if (sku.equals(previousSku))
      balance = balance.subtract(Money.fromPence(20));
    previousSku = sku;
    return this;
  }
}

This feels like a huge hack, and quite a lot of code to write for one failing test. But on the plus side, the test now passes, so I take stock ready to refactor. As before, I have introduced some Connascence of Value, because both the Checkout and the test know about that -20 for the discount amount. This is level 8 (of 9) connascence, so I better remove it before I do anything else. As before, I can fix it quickly by passing it into the Checkout from the test:

@Test
public void discountForTwoAs() {
  Money priceOfA = randomPrice();
  Money discount = Money.fromPence(20);
  Checkout checkout = new Checkout();
  checkout.scan("A", priceOfA, discount)
          .scan("A", priceOfA, discount);
  Money expectedTotal = priceOfA.add(priceOfA).subtract(discount);
  assertEquals(expectedTotal, checkout.currentBalance());
}

As a brief aside, I should mention that this code feels awful to me: All of my instincts and experience are screaming that these Money values should have been passed to the Checkout’s constructor, and not via the scan() method as above. I suspect that’s because I do a good amount of domain modelling in my head as I’m coding. For these blog posts I am deliberately “following the rules”; I have no idea whether this will lead to code I like, and I guess that’s half the fun. Anyway, allons-y…

That last refactoring eliminated the Connascence of Value, which is good. But now I have Connascence of Position instead. That’s because both the test and the Checkout need to agree on the order in which those two Money values are passed into the scan() method. The problem here is that the compiler can’t help, so it would be quite easy for the caller to pass them in the wrong order. While that’s not a big problem for the test, it could be financially disastrous if any other client of the Checkout passed the values in the wrong order.

(Connascence of Position is level 5 — more serious than Connascence of Meaning, but less serious than Connascence of Value. One of the things I find most appealing about connascence is this strict hierarchy: I always know which coupling to address next.)

I can think of a few ways to tackle this Connascence of Position:

  1. Move either Money parameter to the Checkout’s constructor. This would fix the Connascence of Position because there would no longer be a single method with two Money parameters.
  2. Wrap the SKU and its price in a single object. This is attractive, because it has the early scent of a PriceList. This might go some way towards quieting the domain modeller in my head.
  3. Pass the discount via a new method on Checkout. This would remove the Connascence of Position, but would introduce a “setter” — a method that configures the Checkout after it has been constructed. That would be a terrible idea, because it forces the object’s user to remember the order in which these methods must be called. That would be Connascence of Execution Order — level 6 on the scale of 9..
  4. Wrap the discount in a new SpecialOffer object. Wrapping a single object inside another, just to avoid Connascence of Position, seems overkill.
  5. Wrap the two Money values in a dictionary. One of the primary ways of eliminating CoP is to use named parameters. I’m working in Java today, so that isn’t an option. I could fake it by creating a Map<String, Money>, but these two values don’t feel as it they belong inside the same data structure.

Now I see these options, I realise that all of this is premature. I could take the code in any number of different directions at this point, and I don’t have enough information to decide which will be most appropriate. So I need the next test, in order to make this decision in context. I decide to recycle the test, adding a different item between the two As:

@Test
public void discountForTwoAs() {
  Money priceOfA = randomPrice();
  Money priceOfB = randomPrice();
  Money discount = Money.fromPence(20);
  Checkout checkout = new Checkout();
  checkout.scan("A", priceOfA, discount)
          .scan("B", priceOfB, discount);
          .scan("A", priceOfA, discount);
  Money expectedTotal = priceOfA.add(priceOfA)
                                .add(priceOfB)
                                .subtract(discount);
  assertEquals(expectedTotal, checkout.currentBalance());
}

As always, I make this pass without thought of any design aesthetics:

public class Checkout {
 
  //...
  private int countOfAs = 0;
 
  public Checkout scan(String sku, Money price, Money discount) {
    balance = balance.add(price);
    if (sku.equals("A"))
      countOfAs++;
    if (countOfAs == 2)
      balance = balance.subtract(discount);
    return this;
  }
}

It seems I always manage to introduce Connascence of Value! This time, both the test and the Checkout know which product is discounted, and how many items trigger that discount. As before, I fix that by passing the duplicated knowledge via a parameter:

@Test
public void discountForTwoAs() {
  Money priceOfA = randomPrice();
  Money priceOfB = randomPrice();
  Checkout checkout = new Checkout();
  Money discount = Money.fromPence(20);
  checkout.scan("A", priceOfA, "A", discount, 2)
          .scan("B", priceOfB, "A", discount, 2)
          .scan("A", priceOfA, "A", discount, 2);
  Money expectedTotal = priceOfA.add(priceOfA)
                                .add(priceOfB)
                                .subtract(discount);
  assertEquals(expectedTotal, checkout.currentBalance());
}

At this point I can take stock. I have made the Connascence of Position worse, because now the caller also has to know which of those two SKU parameters was scanned and which is the indicator for the special offer. I have also created other connascence; more about that in the next post. For now, Connascence of Position is still my biggest problem, and my options for fixing it are now suddenly much more clear: The three discount-related parameters represent aspects of the same domain concept: a MultiBuyDiscount. I can defeat the Connascence of Position by wrapping those values in a new class:

@Test
public void discountForTwoAs() {
  Money priceOfA = randomPrice();
  Money priceOfB = randomPrice();
  Checkout checkout = new Checkout();
  MultibuyDiscount discount = new MultibuyDiscount("A", Money.fromPence(20), 2);
  checkout.scan("A", priceOfA, discount)
          .scan("B", priceOfB, discount)
          .scan("A", priceOfA, discount);
  Money expectedTotal = priceOfA.add(priceOfA)
                                .add(priceOfB)
                                .subtract(Money.fromPence(20));
  assertEquals(expectedTotal, checkout.currentBalance());
}
public class Checkout {
//...
 
  public Checkout scan(String sku, Money price, MultibuyDiscount discount) {
    balance = balance.add(price);
    if (sku.equals(discount.discountSku))
      discountItems++;
    if (discountItems == discount.discountTrigger)
      balance = balance.subtract(discount.discount);
    return this;
  }
}

As before, by fixing some connascence I have discovered a new domain object and given it a name and a real existence. But as yet, that new object is simply a data container: it has three public fields, and all of the processing that involves them is still in the Checkout. This situation breaks the strong suggestion of Demeter (and it looks pretty smelly too). I’ll need to fix that soon.

However, this post has grown much longer than I like, so I’ll stop here. Next time I will review this code for more connascence…


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 }}