Michael Feathers

Temporal Correlation of Class Changes

How do you know that you have the right architecture for your application?  First of all, we have to define what "right" is, and there are many opinions.  

One worthwhile goal for an application is to have classes that are reasonably independent.  If we have some functional change to make, we should be able to go to a single class and make the change.  If we have to modify several classes it could be because we are introducing a large feature, or it could be because of the way that our application is decomposed.  Adding a field to a model class might require changes in a view, and that's normal.  It's a side effect of separating representation and presentation. 

In many cases, though, we end up making changes in several places because we have faulty abstractions.  If many changes to one class necessitate change to another, it's an important piece of information.  It could be an indication of the code smell that Martin Fowler named 'Shotgun Surgery' in his book `Refactoring: Improving the Design of Existing Code' [Addison-Wesley 1999].  'Shotgun Surgery' is essentially, the code smell that you have you find that adding features requires you to make changes spread across wide areas of the code base.

 The sad thing about 'Shotgun Surgery'' is that we're pretty much left up to our day to day experience to detect it.  We might get the sense that we are touching too many areas of our code, but that is a general feeling.  Do we really know which classes are tied together in our problem space?  Do we know what other classes we are likely to touch when we touch the one we are working on? It turns out that we have all of the information we need to figure this out in our source code repositories.

Representing Code Change

Over the past few months, I've settled on an intermediate form that I use for data I gather from source code repositories.  It makes change analysis and many other forms of analysis rather easy.  The central data structure is a method-change-event.  Each method change event has the following fields:

- type (method add, change, or delete)
- method name (fully qualified with class/modules)
- method body length
- file name
- sha1
- commit date 
- committer

With this information, I can track the changes of individual methods across their entire lifetimes.  I can also do higher level analysis of qualities associated with files and classes.  

Approach

When methods from two or more classes are changed in the same day by the same committer, we can say that the classes are somewhat correlated in time.  When this happens often their correlation is high. For our purposes, it's probably be sufficient to compute the pairwise correlation of classes.  

Given that we have a set of events which represents the state of every method each time it is changed in our project's history, we should be able to find all of the classes that have been touched by each committer on each day.  We can then take all pairs of those classes and throw them into a set.  When we do this for each commit and total the number of times each pair occurs across all of the commits, we should be able to see the relative frequencies of the pairs.  When we sort them, we'll see which classes change most often with each other.

Fortunately, this sort of analysis is rather brief in Ruby.  Here is the whole of the computation after a few extensions to Array and my method change event class:

events.group_by {|e| [e.day,e.committer]}.values   
  .map {|e| e.map(&:class_name).uniq.combination(2).to_a }    
  .flatten(1).norm_pairs.freq_by {|e| e }.sort_by {|p| p[1] }

When you examine these sorts of frequencies, they typically have that power law-ish shape:

Once you have a sorted list of the highest frequency class pairs, you can march through them and see if there are any surprises. Temporal correlation of change across domain objects is particularly informative. Altering the correlation range from a day to a week can be useful too.

Finessing Away Errors

I think I've written about this before but I don't care.  I'm going to keep harping on it.  Error detection and handling should be on the edges of programs, not on the inside.  You shouldn't have to get halfway through your execution to discover that something is wrong.

It's nice to have this sort of philosophy, but how do you put it into practice?  Let's look at an example.  

This morning I was working on some code repository mining code and I needed to group some events.  I knew what to do, I reached for group_by in Ruby.  It's a method on arrays which takes a block and executes for each element in the array.  Every unique return value from the block turns into a key in a hash that group_by returns.  The values in the hash are all of the elements which produced the result value when the block was run on them.

Here's a simple example:

  events.group_by {|e| e.method_name }

When I execute that, I get a hash with method names as keys.  Each key in the hash is associated with all of the events that have that method name.

So, where do the errors come in?  Well, the method names in my system are fully qualified.  "Utilities::Config::search" is an example.  The name denotes a method named search on the Config class in the Utilities module.  Now, suppose that I want to group on the names of classes?

It seems easy enough.  Here's a Ruby function that which will let me do that:

  def class_name method_name
    method_name.split('::')[0..-2].join('::')
  end

  events.group_by {|e| class_name(e.method_name) }

The function takes the name and splits it on '::'.  Then it takes all segments except for the last and joins them up again.  That should be our class name.  There is a problem, though.  It doesn't work for methods without a class.  It returns nil because [0..-2] on an empty array is undefined.

Hmm.. What can we do?  Well, my first tack was to just return "" from class_name when there was only one segment:

  def class_name method_name
    return "" if method_name.split('::') <= 1
    method_name.split('::')[0..-2].join('::')
  end

but then we have to get rid of that entry:

  events.group_by {|e| class_name(e.method_name) }.select {|k,_| not k.empty? } 

More than a bit ridiculous, eh?

Is there a better solution?   Exceptions to the rescue!!   No, just kidding.

Whenever you have an error, it pays to see if you can turn it into a normal case.  In this case, it's very easy.  It turns out that all methods in Ruby are part of a class.  When you don't see a class in the source file, Object is the implicit class.

Here's the fix:

  def class_name method_name
    return "Object" if method_name.split('::').count <= 1
    method_name.split('::')[0..-2].join('::')
  end

Now all methods that are defined without a class are binned under Object.  For instance, our class_name method will be seen as Object::class_name.

It might seem that we got lucky here, that the design of Ruby gave us an easy default.  Well, the truth is more sublime.  Ruby is playing the same trick that we are.  In essence, Ruby is saying that methods that don't appear to have a class aren't really special.  They are part of Object.   Here's the nice thing.  If Ruby hadn't played this trick, we could've.  We could've created a name like "no_class" and used it as a bin for all methods that aren't "part of" a class.     

Now that we have our class_name function, we can group away in an error-less way:

  events.group_by {|e| class_name(e.method_name) }

And we can compose without checking for error cases:

  events.group_by {|e| class_name(e.method_name) }.values.map(&:count).freq

because there are none.