A Tough Call

This other day I’m like coding and stuff when a co-worker walks by and asks to have a word. It turns out he needed some advice after he got himself in a pickle by checking in slightly more code than was supposed to. Something about refactoring a bunch of code instead of just fixing the dozen or so bugs he was assigned, which resulted in some frowns.

At this point you’re probably wondering why he came to me, given that I have nothing to do with his project. To that I would say I sometimes play the role of the bartender among developers with the exception that I don’t serve drinks and expect no tips. Hopefully that make sense.

So it turns out this guy set out to fix those bugs when he realized he’s dealing with a bunch of spaghetti with a full can of past-expiration-date bolognese sauce ton top. Being a fearless engineer, he took matters in his own hands and decided to refactor the offending code. But there’s trouble in paradise – the refactorings didn’t go too well with the management.

As we talk about this, I can’t help remembering so many past instances when we had some prototypical loose cannon make frivolous changes all over the place and breaking a bunch of stuff. The worst part was when I got stuck cleaning up after them. But there were also times when that was the right thing to do and it brought about a lot of good. The question is – which is it this time?

I put on my metaphorical referee uniform and tried to reason through this. Refactoring can definitely be risky and its benefit must be weighed against the risk. However, after deliberating for about 10 minutes I called the move – legal. The deciding factors were:

  1. This engineer stands behind his work. He expressed the willingness to “work all night” to rectify any issues that may arise as a result of this.
  2. He knows his s*%t. He made a convincing case why that old code was no good (monolithic methods, incorrect abstractions, etc), and why his code is likely to be more maintainable.
  3. He has a history of good coding practices.

In addition I decided to give his superiors yellow cards for taking the short-sighted approach of “if it aint broke don’t fix it” because they don’t know enough about the code in question (or code in general IMO) to make that call. Surely there are cases when it’s better not to fool around with messy code when it’s fully contained and debugged, but this was not one of them.

As a result of my decision, nothing happened. This was in part because we didn’t tell anyone so in the end it was just two dudes talking about some work stuff. For what it’s worth, I remain very pleased with my decision.

Advertisements
Post a comment or leave a trackback: Trackback URL.

Comments

  • Ken Franqueiro (@kfranqueiro)  On February 3, 2012 at 8:50 pm

    I have a feeling I know which engineer you are talking about, and as a result I kind of cringed a couple of times while reading this, because I know the person in question is a guy who absolutely means well and is tremendously dedicated to writing good, logical, maintainable code.

    That said, I can somewhat understand why management would have an adverse response, and it comes from an important factor here that, while mentioned in passing in your post, is not necessarily given enough weight: risk.

    Regardless of how awesome a developer is, refactoring any amount of code is always a risk for regressions. One vector attributing to this risk is scope. Given the spaghetti-like nature of the code in this particular instance, it’s likely that the refactor spread across various packages, components, and/or concerns. In an ideal scenario, it is immensely important that such refactors be conducted in the smallest chunks possible, and that reviews be conducted and regression tests be run after each step along the way.

    Failing this, you will be stuck facing the situation that you have an enormous amount of code to check-in, all of which may have fallen out of date with concurrent developments by other team members, which have to be merged; then you also have to get your code reviewed, and since it has ended up sprawling across so many different components, this may involve several rounds of mostly-but-not-quite-independent reviews with several different team members. Then comes the worst part: as regressions do pop up in the tests, rather than being able to say “oh, well I only touched these 3 classes really yesterday, so let’s have a look”, you are faced with a huge pile of changes to pick through to find the culprit.

    In addition to the obvious dangers of a broad refactor, however, the other factor I imagine must have come into play – and mostly why I can empathize to a degree with the management – is impact on schedule. I don’t necessarily mean this as in, “hey, this engineer has been spending all his time doing this when we would like him to be doing other stuff” – though there’s probably that too. What I mean is, management is probably already worried sick about trying to release a product on time, and all of a sudden, SURPRISE REFACTOR OF ALL THE THINGS happens. This takes a situation which was probably just barely managing to keep the number of variables in check, and dumps a whole new bucket of variables on it. If the sky was falling before, then it’s anybody’s guess which direction the sky is moving now. Wait, we’re still in orbit, right?

    I don’t mean to downplay the importance of cleaning up messes and improving code maintainability; I’m certainly all for that. Do I have faith that the developer’s refactor will ultimately lead to fewer headaches down the line? Absolutely.

    However, refactoring within a project of any significant size is never something to be taken lightly, as it will undoubtedly consume a significant amount of resources (and almost definitely more than the developer will initially estimate). A refactor is certainly not something to simply barge into of one’s own accord without checking with a few teammates or even superiors first. (I’m not saying that’s what happened, since I don’t know the particulars, but my point is that any thought to refactoring code for a project on a tight schedule should be weighted with a significant barrier to entry, so to speak.)

    Ultimately, there are instances where it is just not the right time for a refactor. Of course, therein lies the problem – in many work environments, there is no such thing as a Good Time to refactor. I’m sure you probably heard me repeat that myself a few times. 😉

    • vvakar  On February 3, 2012 at 9:14 pm

      Having the inside scoop surely adds depth to the conversation 😉
      Given the personality and skills of the protagonist, I tend to be biased in favor of refactoring just because I’ve pulled a few stunts like that in the past and things somehow worked out ok (with the help of some after-midnight damage control sessions, but I took that as being part of the whole experience). This approach, in addition to the risks, has the side effect of the person taking personal ownership of their work and staking their reputation on it. For some people, that can turn a dull and protracted trip through Burnoutville into a thrilling and energizing ride. But, you still need the right people with sufficiently balanced personalities to keep it real.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: