Connascence of Position

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.

Continue reading

Connascence of Meaning

This is part 2 of a short series of posts in which I explore TDD using only connascence as my guide. In the previous article I wrote a test, made it pass, and then refactored away the strongest coupling. That coupling took the form of some Connascence of Value between the test and the Checkout. Later, after the excitement of publishing the post had died away, I realised there was still some non-trivial connascence in the code. Today it’s time to fix that.

Continue reading

Connascence of Value

Connascence is a way of describing the coupling between different parts of a codebase. And because it classifies the relative strength of that coupling, connascence can be used as a tool to help prioritise what should be refactored first. This is the first in a short series of posts in which I test-drive a well-known kata, attempting to use only connascence as my guide during refactoring.

Continue reading

Eliminate many conditionals with this one weird trick

Recently I attempted to classify the conditionals in software according to where in the code they originate. The first category of these was: Checking a value returned to me from code I own. A large proportion of these can be eliminated quite simply.

Imagine we want to begin reading a file at the last saved read position, which may be null if we haven’t read anything yet:

var readOffset = fileReader.GetSavedReadPosition();
if (readOffset == null)
  readOffset = new ReadPosition(fileReader, 0);

This code violates almost every rule in the book:

  • There is duplication, because both the caller and the callee have special code for the “no saved position” branch.
  • There is Connascence of Meaning, because both need a common understanding of what the special null value means.
  • We are violating the Tell, don’t Ask principle because we are making a decision on the basis of a value returned from a call to a different object.

All in all, this code has problems — and yet I see code like this everywhere I look. So, what to do?

Let’s look at this code from the Connascence point of view. The problem is the null value representing the special “not saved” case: both the caller and the callee have to agree to use null to mean that (hence “Connascence of Meaning”). Now, connascence becomes stronger with increasing distance. Our example smells strongly because the two connascent code fragments are in different classes; thus we could weaken it if we can bring the endpoints closer together.

Currently, the fileReader decides what to return when it has no saved read position, and the client code has to cope with that decision. What if, instead, the client code decides what it would like to get back in that case; what if it could simply tell the method what to return if it can’t do the thing we’re asking:

var readOffset = fileReader.GetSavedReadPosition(0);

Now the connascence has disappeared, and the conditional has disappeared with it. It’s as simple as that.

Many frameworks and APIs offer this kind of default return value parameter. For example, the standard Ruby Hash class provides this feature via the fetch method.

But what if there’s no sensible default value that we can pass into the called method? For example, consider the following case:

var username = auth.GetPrincipal();
if (username == null)
  throw new UsernameNotFound();
auth.SetPassword(username, password);

We still have Connascence of Meaning due to the null return case; and we don’t want to push the error handling (or whatever) down into the authentication object, because different callers may want to handle the situation differently. But we can take a leaf from the default parameter book, and have the authentication object execute code for us:

var throwIfNotFound = () => throw new UsernameNotFound();
var username = auth.GetPrincipal(throwIfNotFound);
auth.SetPassword(username, password);

Again, the null value has disappeared, and so has the Connascence and the conditional. For the sake of symmetry we can often also dispense with the return value altogether and pass in both code branches to the callee:

auth.WithPrincipal(
  (username) => auth.SetPassword(username, password),
  () => throw new UsernameNotFound());

lambda1

Pretty much every modern language supports this kind of code now:

  • Functional languages allow us to pass in functions to do the job; likewise Javascript;
  • C and C++ let us pass function pointers;
  • C# lets us use lambdas via Linq; Ruby lets us pass in lambdas, and even C++ (thanks @mmeijeri) and Java 8 now allow lambdas.

Some even let us use named parameters to make the options a little clearer; for example, we might write the above example in Ruby thus:

auth.withPrincipal(
  if_known: lambda {|username| auth.set_pass(username, passwd)},
  if_unknown: lambda {raise UsernameNotFound.new})

It’s as simple as that. By adopting these approaches it is possible to simplify a lot of code, effectively removing the connascence and the duplicated conditionals from the caller and the callee.

A problem with Primitive Obsession

By an odd coincidence, a number of the teams I work with have been looking at the Primitive Obsession code smell recently as part of the software habitability and craftsmanship training programmes I am currently running. As we have explored legacy code looking for examples of the smell, I’ve noticed a tendency to enthusiastically label every primitive in any method signature as smelly. This certainly demonstrates Primitive Obsession, but mostly on the part of the reader of the code, rather than in the code itself.

I so wish Fowler and Beck hadn’t used this particular name, because it seems to spread as much confusion as enlightenment. In my book I tried renaming it to Open Secret, but that was a very small voice lost in the noise. So hear my plea: please, please stop using the term Primitive Obsession — let’s talk about Connascence of Meaning instead.

I think it is important to recognise that not every primitive in a method signature represents (bad) Connascence of Meaning: Firstly, the problem is significantly weaker when the method is private (ie. it can only be called from inside the same object). That’s because connascence becomes stronger with distance. Inside a single class we expect a lot of connascence, because otherwise the class is probably not a cohesive unit of responsibility. It’s a fairly weak smell when two methods in a class both know how to interpret the meaning of a primitive.

And secondly, recall that this smell is mostly about the way domain concepts are represented and understood. If the primitive is passed straight through our code without being parsed in any way, again the smell is much weaker because we are not attempting to intuit any Meaning for ourselves. For example, if you’re working in a domain in which time is always given to you as a long, and you never do anything with it except store it or pass it on, then there is certainly Connascence of Meaning between the libraries you’re using, but I would argue that there’s probably none in your own code.

In general I’m finding that connascence is better than code smells as a tool for discussing coupling. And in particular, it seems to me that Primitive Obsession creates the wrong impression and makes the concept harder to learn.

On boolean externalities

This week @avdi wrote a really interesting blog post in which he expounds the woe induced by trying to debug a stack of methods that return booleans to each other. I couldn’t get Disqus to load on the post itself, so my response is below. Go ahead and read Avdi’s article now; take your time, and think about what you would change in his code; I’ll wait here until you’re ready.

A lot of the comments on That Twitter suggest that this can be “fixed” by returning things that are more complex than simple booleans, including switching to a different language to make that easier. I think most of them are missing the point. I think this is a representation problem. Let me explain.

The code as it stands uses several booleans — I counted around six, and I suspect there are more in the code branches we weren’t shown. In order to figure out whether the user is allowed to view an episode, the code effectively rummages through the user’s past, looking for a confluence of events that combine together to give the user that permission. And so when the code doesn’t do what was expected, poor Avdi then has to do the same, only using the debugger or logging statements. The problem therefore, it seems to me, is that the current state of the user is only represented indirectly.

It seems to me that this code breaks the second rule of Simple Design, in that the domain is not represented faithfully by these booleans. I would be willing to bet that not every one of the 2n possible combinations of values of these booleans is possible. Indeed, it seems likely that there are only a handful of states in the lifecycle of a user. So to use a group of booleans to represent these states is to misrepresent the structure of the domain: The representation permits many more possibilities than can actually occur, and leaves the job of figuring out the current state to a bunch of other methods scattered through a couple of different objects. Another way to express this might be to say that the code shows Connascence of Meaning (or Algorithm), because numerous methods need to know something about how the user’s state is represented.

So my solution would be to create an explicit state transition model for users, and then to represent that in the code using a state field on the user. It would then be possible to produce errors such as “User X cannot do Y because s/he is in state Z”. It also opens up the possibility of using the State pattern, so that the user would hold a reference to an object representing its current state. Such an object might know how and when the user arrived in this state, and have code that validates the user’s state changes. It might even provide the valid onward transitions as, say, Command objects.

So that’s my approach, and I’ve been finding it more and more useful recently to solve modelling problems such as this. The question is: is this applicable in Avdi’s case; does the code we haven’t seen support this design…?

How to measure habitability

I have argued that in order to measure the habitability of a codebase, we should focus on attempting to measure how well the code conforms to the Once And Only Once rule. So what does that rule mean? It means that every important concept in the domain is represented (named) explicitly — not buried inside another concept (Once), and not spread out in the space between several other concepts (Only Once).

Once:

How can we write a tool that will detect when a concept is buried inside another? Well, let’s flip that question around: How can we tell when a concept contains another buried inside it? I’m not sure that pure size is a reliable guide, because design concepts have no standard size. Some will require only a few lines of code, while others will need dozens. But it is probably not unreasonable to assume that a file which is significantly larger than the others in a codebase is probably harbouring too much responsibility.

What about complexity? It is usually true that a class with many conditionals or many levels of indentation is asking to be simplified. And very often the best response to over-use of branching is to break out the Strategy pattern, which gives explicit names to two or more of the responsibilities hidden inside the original class.

Lopez et al, in Relevance of the Cyclomatic Complexity Threshold for the Java Programming Language [pdf], propose that the McCabe complexity of object-oriented software is lower (less than half) that of procedural software. While their study does not appear to take the degree of habitability into account, it seems suggestive to me that some measure of branching might be a reasonable reflection of whether a class is pregnant with unnamed concepts.

Efferent coupling [pdf] may also be indicative here. It seems intuitively correct that a class that has to import/require many others is probably doing too many things.

Only Once:

How could we write a tool that will detect when a concept is spread out in the space between two or more others? Well, again the presence of branches may be an indication of Connascence of Meaning (think of the Null Check and Special Case code smells). Primitive Obsession is also a symptom of Connascence of Meaning, but I suspect we could only detect that automatically in statically typed languages (without actually running the code).

Again, efferent coupling may be indicative. When two classes both import/require another, there may be some Connascence of Algorithm going on — or at least a missing abstraction wrapping the imported stuff.

So while I have no evidence, my instincts tell me that we may be able to get a decent hint at habitability by finding some workable combination of coupling and branching measures. Soon I will begin exploring these ideas with code samples in a variety of languages, to see whether I can work up a metric (or a system of metrics) that can predict whether I will like the code…

Choosing what to measure

Suppose we want metrics that help us write “better” code — what does “better” mean? I’m going to fall back on Extreme Normal Form, more commonly known as the four rules of Simple Design. These say that our priorities, in order, when choosing what to improve are that the code should:

  1. be correct and complete
  2. express the design clearly
  3. express every concept once and only once
  4. be small

Let’s agree to call code that meets these criteria “habitable code”, in that it is easier to understand and maintain than uninhabitable code. How can we design a metric that rewards the creation of habitable code?

Now it seems to me that most teams have processes in place for ensuring correctness and completeness. These might be requirements traceability procedures, suites of (manual or automated) tests, QA procedures, user testing, focus groups, etc etc. The first rule of simple design helps to ensure that we are only working on valid code, but doesn’t help us differentiate “better” code. Only the remaining three rules can help us with that. In a sense, the first rule can be taken as a “given” when our projects are set up with appropriate QA processes and procedures in place. I will therefore not consider test coverage when searching for a metric (indeed I believe that test coverage is an unhelpful measure in the context of code habitability).

Let’s move on to the second rule of Simple Design. Joe Rainsberger re-casts this rule as No bad names, because in the huge majority of current programming languages the names we give to variables, functions, classes etc are our primary (often only) means of expression in the code. I believe it is impossible to directly measure how well code expresses intent, or whether the names chosen are “good” in any aesthetic sense. But the beauty, coherence and honesty of the names chosen by the programmer is only one aspect of applying this rule. Implicit in the statement “No bad names” is the notion that the programmer has chosen to give names to the “correct” set of entities. That is, choosing the things to reify and name in the code is at least as important as, if not more important than, choosing the best name to give those things.

This is where rules 2 and 3 begin to trip over each other (and perhaps why some authors place them in the opposite order). Consider for example the Primitive Obsession code smell. This occurs when two different classes communicate using (messages containing parameters of) primitive types (string, integer, array etc). The primitive(s) used for the message parameter(s) represent some domain concept that must be known to both classes; but this code is smelly because both classes must also understand the particular representation chosen for that concept in this particular message. The two classes exhibit Connascence of Meaning, and the third rule of Simple Design is violated because the representation of the domain concept is understood in two different places in the code. But we could also consider this to be a violation of rule 2, because that domain concept has not been reified into a class and named — we have a missing name.

Based solely on this type of logic, I am therefore going to boldly assert that any metric that gives a good indication of the degree to which rule 3 is followed, will also be a good indication of how well rule 2 is followed (allowing for the fact that we cannot measure aesthetics). So as a proxy measure I claim we can focus solely on the Once And Only Once rule as a measure of the habitability of our code.

The problem with code smells

Like most developers I know, I have used code smells to describe problems in code since I first heard about them. The idea was introduced by Kent Beck in Fowler’s Refactoring back in 1999, and has taken root since then. The concept of code smells has several benefits, not least the fact that it gives names to ideas that were previously only vague. Having a list of named code quality anti-patterns helps all of us discuss them on the same terms.

But while I was writing Refactoring in Ruby with @wwake, and writing Reek at the same time, I began to feel a little uneasy about them. I was never able to put my finger on exactly why that was, or what I was uneasy about, but the feeling never went away. This year I think I have finally understood what I think about code smells, and why I think we can do somewhat better. So before reading on, take a moment to list the things you don’t like about them. Then let’s compare lists. Go ahead, I’ll wait.

Ok, done that? Here are my current reasons for wanting a different tool for describing code quality:

  • The names aren’t all that helpful for people unfamiliar with the concepts. If you had never heard of them, what would you make of Feature Envy, Shotgun Surgery, Data Clump etc? Sure, the names are memorable, but that only helps with hindsight, for people who have taken the time and trouble to investigate and learn them.
  • Some code smells can overlap. For example, I’m often unsure whether I have seen Feature Envy or Inappropriate Intimacy, Divergent Change or Large Class. There’s a sense in which this doesn’t matter, of course; but it undermines their use as a communication tool.
  • Some of the smells can be subjective or contextual, leaving the quality of the code open to interpretation. For example, how large is a Large Class or a Long Parameter List?
  • Some of the smells apply only under certain circumstances. For example, a Switch Statement is perfectly fine at the system boundary when we are figuring out the type of an incoming message, but often bad news when it represents a type check on code we own. And it can be acceptable at the system boundary to grab the fields of a Data Class in order to display them, while elsewhere that might be seen as Feature Envy.
  • The list of code smells is not canonical; different people have added their own smells to Beck & Fowler’s original list. This situation is even worse when it comes to smells in unit tests; try looking for a canonical list of test smells and you’ll find no consensus whatever. In my opinion, this fact alone completely undermines the idea that code smells form a pattern language for describing code quality.
  • There are no clear and obvious code smells covering some dynamic problems, such as the coupling between variables whose values depend on each other, or the problems introduced by mutable objects.

Did you have the same list, or something similar?

So, what can we do about it? I think the answer lies with Connascence. This is an idea introduced by Meilir Page-Jones in two books in the 1990s, and later popularised by @jimweirich in a series of conference talks. I’m not going to cover Connascence in detail here — you can find it all for yourself by reading @jimweirich‘s articles or looking at my summary slides. I just wanted to take a moment to write down my current opinions about code smells. I’ll probably write in more detail about Connascence in the coming weeks, but for now what do you think?