Adherence to DRY (“Don’t Repeat Yourself”) does not necessarily preclude repetition of code. In the endless struggle to refactor, the entropy we are trying to reduce is not in the raw text of our source code; it is in our business logic, which (in applications with little or poor testing) is often uncodified. Sometimes, refactoring can hamstring our code, and when done naïvely it can be a source of technical debt, rather than an antidote thereto.

Many programmers would agree with the following statement:

[I]f you use copy and paste while you’re coding, you’re probably committing a design error. Instead of copying code, move it into its own routine. Future modifications will be easier because you will need to modify the code in only one location. The code will be more reliable because you will have only one place in which to be sure that the code is correct.

— http://www.stevemcconnell.com/ieeesoftware/bp16.htm

I subscribed to this philosophy for the first few years of my real-life programming career. But recently I’ve questioned it, driven by experience working with clients whose requirements change (that is, all of them).

Identity

There are often situations where a code path is required to be the same as another for an explicit reason. Take the Twitter API documentation, for example:

In version 1.1, we’re requiring applications to authenticate all of their requests with OAuth 1.0a. — Overview: v1.1 of the Twitter API

The whole OAuth protocol setup, and addition of necessary tokens to each request, would no doubt benefit from being written once and only once. Bugs and security issues that arise will only need to be fixed in one place, and OAuth is mandated by the specification for all calls to the API. If you were to look at the call trace of a process requesting tweets for a given user, and that of one searching for a hashtag, the OAuth-related logic in common between the two would be identical in the philosophical sense.

When Refactoring Goes Bad

But just because two processes have to do the same thing, it doesn’t mean the business cases represented by those code paths are necessarily identical. Let’s say we’re building a very basic Twitter clone, with both a global feed displaying all tweets on the site, and individual user timeline pages showing just tweets from that user. Well, we might start out like this:

  # urls:
urlpatterns = patterns('twitclone.views',
    url(r'^$', 'global_feed'),
    url(r'^(?P<username>\w+)/$', 'user_timeline'),
)

# twitclone.views:
def global_feed(request):
    tweets = Tweet.objects.all()
    return render(request, 'global_feed.html', {'tweets': tweets})

def user_timeline(request, username):
    tweets = Tweet.objects.filter(user__username=username)
    return render(request, 'user_timeline.html', {'tweets': tweets})

This is reasonable enough. The overzealous refactorer would look at code like this and say: “Configuration over code—I can see similarities between two different functions!” and replace it with this:

  # urls:
urlpatterns = patterns('twitclone.views',
    url(r'^$', 'tweet_list', kwargs={'template': 'global_feed.html'}),
    url(r'^(?P<user__username>\w+)/$', 'tweet_list', kwargs={'template': 'user_timeline.html'}),
)

# twitclone.views:
def tweet_list(request, **kwargs):
    template = kwargs.pop('template')
    tweets = Tweet.objects.filter(**kwargs)
    return render(request, template, {'tweets': tweets})

Said developer congratulates itself with a pat on the back for a job well done.

Then a product owner says that a profanity filter needs to be added to the global feed, but not the individual user feed. The freshly-refactored code now gets a configurable profanity filter switch:

  # urls:
urlpatterns = patterns('twitclone.views',
    url(r'^$', 'tweet_list',
        kwargs={'template': 'global_feed.html', 'filter_profanity': True}),
    url(r'^(?P<user__username>\w+)/$', 'tweet_list',
        kwargs={'template': 'user_timeline.html', 'filter_profanity': False}),
)

# twitclone.views:
import itertools

def tweet_list(request, **kwargs):
    template = kwargs.pop('template')
    filter_profanity = kwargs.pop('filter_profanity')
    tweets = Tweet.objects.filter(**kwargs)
    if filter_profanity:
        tweets = itertools.ifilter(lambda t: not t.is_profane(), tweets)
    return render(request, template, {'tweets': tweets})

Because the profanity filter is implemented in Python, we have to sacrifice the QuerySet API for the tweets object, and hope that the global_feed.html template doesn’t rely on any QuerySet methods. But we maintain a single function, with the options configurable through the URLconf, so we’re still ‘DRY’, right?

Now a requirement is introduced that the user timeline (but not the global feed) needs to be paginated. Again, our solution grows more legs:

  # urls:
urlpatterns = patterns('twitclone.views',
    url(r'^$', 'tweet_list',
        kwargs={'template': 'global_feed.html', 'filter_profanity': True}),
    url(r'^(?P<user__username>\w+)/$', 'tweet_list',
        kwargs={'template': 'user_timeline.html',
                'filter_profanity': False,
                'tweets_per_page': 20}),
)

# twitclone.views
import itertools

def tweet_list(request, **kwargs):
    template = kwargs.pop('template')
    filter_profanity = kwargs.pop('filter_profanity')
    tweets_per_page = kwargs.pop('tweets_per_page', None)
    if tweets_per_page is not None:
        page = request.GET.get('page', 1)
        offset = (page - 1) * tweets_per_page
        tweets = Tweet.objects.filter(**kwargs)[offset:offset + tweets_per_page]
    else:
        tweets = Tweet.objects.filter(**kwargs)
    if filter_profanity:
        tweets = itertools.ifilter(lambda t: not t.is_profane(), tweets)
    return render(request, template, {'tweets': tweets})

I find this code to be unnecessarily confusing, and I believe most other developers would agree. I strongly dislike putting configuration of any sort in the URLconf unless it directly impacts the URLs, and we now have a single generic function in the place of two specific ones, which has the potential to grow combinatorially in the future as more options are added.

Furthermore, the business spec talks about a global feed, which is profanity-filtered but not paginated, and a user timeline, which is paginated but not profanity-filtered. Our generic code implements both cases, so it would seem it meets the spec, but it also implements two cases which aren’t called for by the specification. When another developer (or even the same one, in six months’ time) reviews this code, it will be unclear whether the cases of ‘filtered and paginated’ and ‘neither filtered nor paginated’ are intended but untested, or unintended cases which nevertheless must now be supported for the sake of backwards-compatibility. The code has been parametrized to the point where you can no longer determine if anything else relies on it behaving a certain way.

This may come across as a straw man argument, but I’ve worked in many a situation isomorphic to this—most frequently when I’ve been called in to deal with a mess after the old developers abandoned ship.

Equality

The reason two specific functions were replaced with an unnecessarily-complex generic one was because our developer mistook equal code for identical code. Code paths which happened to be accidentally the same were viewed as being essentially the same, and thus a Huffman coding was applied to the source without attention to the business specification—which, remember, may not even exist in codified form. As a result, when the product owner asked for what should have been an isolated change to an uncoupled part of the information architecture, technical debt was created.

A Better Solution

Added after @danso on Hacker News asked for an example of how the non-refactored way would be better.

Keeping the two functions separate:

  # urls: (still the same)
urlpatterns = patterns('twitclone.views',
    url(r'^$', 'global_feed'),
    url(r'^(?P<username>\w+)/$', 'user_timeline'),
)

# twitclone.views:
def global_feed(request):
    tweets = Tweet.exclude_profane()
    return render(request, 'global_feed.html', {'tweets': tweets})

def user_timeline(request, username):
    tweets = Tweet.objects.filter(user__username=username)
    # You could put the `per_page` argument in your Django settings as
    # `USER_TIMELINE_TWEETS_PER_PAGE`, if you're that way inclined.
    tweets = paginate(tweets, 20, request.GET.get('page', 1))
    return render(request, 'user_timeline.html', {'tweets': tweets})

def paginate(qset, per_page, page_num=1):
    return qset[(page_num - 1) * per_page:page_num * per_page]

# twitclone.models:
class Tweet(models.Model):
    ...
    # Better implemented as a method on the QuerySet, but this'll do.
    # See: https://github.com/zacharyvoase/django-qmethod
    @classmethod
    def exclude_profane(model):
        for tweet in model.objects.iterator():
            if not tweet.is_profane():
                yield tweet

Obviously there’s more going on here than in my straw man example. But the views are simple, and far more easy to debug, because rather than dealing with an implicit, dynamic configuration object (in the form of the kwargs dictionary), there’s a single, simple path through the code.

In terms of testing, you would ideally unit test both the paginate function and the exclude_profane classmethod, and do high-level behavioral testing on the global_feed and user_timeline views to ensure they met their contract, and there would be no ambiguity due to the combinatorial possibilities introduced by the configuration object.

When Tools Make It Worse

I want to draw attention to a very simple case of where this affected an application I worked on, because it was enabled and encouraged by Django: you can specify an ordering property on a model class, and all queries for that model will have the ordering implicitly applied to them. So let’s say you’re reading through your code one day, and you notice the following everywhere:

  def view(request):
    tweets = Tweet.objects.filter(...).order_by('-created_at')

The principle of Not Repeating Yourself kicks in, and you work on ‘cleaning up’ your code by removing all those pesky order_by() calls, replacing them with this:

  class Tweet(models.Model):
    ...
    class Meta:
        ordering = ['-created_at']

Over the next six months, your Twitter clone becomes a big hit, and you’re storing tens of millions of tweets. However, many of your queries are slow, and Postgres query stats indicate that applying ordering to every single tweet query is slowing your site down. So you want to remove that ordering property—but you can’t, because you’ve now lost track of which methods necessarily rely on tweets being ordered, which ones don’t care, and which ones implicitly need the ordering to work, but you can’t detect breakages because the ordering is added by default. No application’s tests are perfectly comprehensive (except perhaps SQLite), so you can’t be certain you won’t break everything by removing the property.

This is how Django’s ordering property invites us to shoot ourselves in the foot.

The Lesson

I’d like coders to recognize the relationship between the business specification, information architecture and source code, and use that to inform smart refactoring decisions, rather than basic gzipping of code.

You can discuss this article on Hacker News.