Skip to content

If my code stinks, I'd rather you just told me.

June 6, 2009

One of the phrases that has stuck with me since my Agile lectures was that “Agile highlights shortcoming in your development process; you need to be prepared to accept this”. It was introduced in the wider context of getting teams and organisations to accept Agile processes; people need to understand that Agile will show them the mess that they’re in, and only then can they get on with cleaning it up.

But it also applies on the micro-level. Agile practices should show me where I’m going wrong; only then can I hope to do better next time. Unfortunately, a lot of the tools and processes that we use from day to day don’t work like that out-of-the-box. Take import handing in the Eclipse IDE, for example…

By default, Eclipse will collapse the import statements at the top of a Java source file to a single line, with a “+” button next to it for you to expand if you so wish. Which, of course, means that, in general, you never look at them unless there’s a problem.

But even if there isn’t a problem in the traditional sense, the state of my imports can be a pretty strong indicator of Code Smell.

If my class has:

  • too many imports,
  • imports from multiple packages, or
  • imports with ugly package names,

then you can be pretty sure that my code’s beginning to whiff.

  • Too many imports implies that my class is dependent on (or, at least, interacting with) too many objects. Am I sure that this class has only one responsibility?
  • Imports from multiple packages implies that I’m cobbling together bits of the system that really ought to be kept apart. Perhaps I need to decouple my code?
  • Imports with ugly names implies that I’ve chuffed something up in the class that I’m importing. Maybe it doesn’t belong in the package I’ve put it in? Maybe I shouldn’t be trying to get hold of that inner class?

Sure, in some cases any one of these “smells” might be perfectly acceptable. But, hey, Eclipse, that’s my call, not yours. Don’t collapse my imports! How can I monitor Code Smell if you’re hiding it from me?

And, yes, I know that it’s just one checkbox in the preferences, but I’m trying to make a broader point here. As programmers, we’re responsible for all of the code we write, including the stuff that the IDE puts in for us. It’s great that Eclipse will pull in my imports for me, and organise them on Save, but hiding them away is, for me, just that step too far.

Advertisements
4 Comments leave one →
  1. June 8, 2009 10:05 pm

    Adopting Agile methods makes the pain more prominent and earlier in the project lifecycle. Also, once uncovered it’s difficult to hide it again, so the practices force you to deal with it. This can be quite unpopular. I think it’s a relief though to have dealt with issues and the pain is worth it.

    In our daily standups we’ve started including the question:

    On a scale of 1-10 how confident do you feel that we will be done on time?

    The first time I suggested we use this, nobody gave a 10, most were about seven. What really surprised me though was how relieved and happy people looked to have been able to express their doubts. Facing up to your fears, with others, is enabling, and not as bad as you’d expect.

    I’ve been using PMD and FindBugs to give me feedback. They follow rule sets to give suggestions for improvements. I start by using all the rules and gradually removing ones I don’t appreciate. They include some regarding unused or redundant imports and classes having too many methods.

    Today I discovered Glean which pulls together lots of metrics, and might fit in well with your Hudson experiments if you’re using Java.

  2. June 9, 2009 2:12 pm

    Hi Pete,

    What sort of scores do devs give in the stand-up now? What sort of variance? Does everyone just give 7? I’d’ve thought that 1-to-10 is too wide a range – scores out of 5? 3? But it sounds good, and a lot better than the nothing we do now…

    There was a lot a talk about estimates in the Agile lectures; the best/most popular way seemed to be a three-point estimate; best-case, expected, and worst-care.

    I hadn’t heard of Glean; in general, if it’s not a Hudson plug-in then I’ll never hear about it, nor have the time to investigate and/or install it (“Make the easiest thing to do the right thing to do”, right?).

    We already use the Checkstyle, Emma, FindBugs, Task Scanner and Warnings plug-ins (but at the moment only Checkstyle has the power to fail the build). We’ve also just installed the Continuous Integration Game plug-in, which scoops up all the various metrics and gives developers an overall score depending on what they break and what they fix. It also hooks into the PMD and Violations plug-ins, so I ought to install those too.

    Uncle Bob wrote about metrics quite recently. I don’t know about the Braithwaite Correlation but there’s definitely a Crap4J plug-in I can install.

    The problem with all these metrics is that no-one (on our team) looks at them; apart from Checkstyle, because it can fail the build, and Warnings, becuse it’s usually zero and the addition of any warnings is obvious. It’s a shame, because when I do find the time to go through the FindBugs list, I find plenty of good pointers to improving the code. My plan is to do the same as we did for Checkstyle; set a limit on the number of warnings allowed, and then lower that limit as bad code is cleaned up (exceeding the limit fails the build).

    Again, a post about our Hudson setup is *another* thing that I’ve got on draft… maybe I should just repost this comment?!

  3. June 9, 2009 9:33 pm

    Today I gave a 6 that we’d be finished by tomorrow lunchtime and everyone else avoided answering the question. Make what you will of that.

    5 or 3 would be good. An odd numbered rating system is great because you can’t give 2.5 stars. Unless you’re lovefilm.com. I wonder whether an executive said ‘But this film is rated 4.5 stars, why can’t I give it 4.5 stars too?’

    It is a lot better than nothing though.

    Warnings does feel useless as it is always zero, but actually that’s the whole point – it’s obvious when it changes.

    We had 12 failing tests in the trunk for over 3 weeks but nobody noticed because nobody runs the tests. It drove me potty though (pottier?). It only took me an hour to fix them (after a few hours research into the unfamiliar techniligy (c:)

    I’ve been doing pair programming for the last 6 work days and it has been a pleasure. That makes about 8 total professional days experience. It’s the best feedback.

Trackbacks

  1. A TDD talk from The Agile Workshop – dantwining.com

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: