Recently I’ve come across my Ruby code from 13 months ago. The code regarded my very first task in a project, while I was about to leave the project. Those 13 months were enough to spot right away that the code was, well, bad.

OK, it had some nice parts. It’s proved its correctness in the wild, had nice test coverage and was fairly understandable. However, the first word that came to mind when I was re-reviewing it was cluttered.

Just to be clear – I like the situation! If you don’t like your code from ago, it means progress! 13 months of teamwork must have changed my mind.

I’ll confess five sins of the code and show some advice from today’s point of view.

(Unfortunately, I can’t show the source code, so I’ll be speaking in generalities)

puts in production code

That just happened. I had no better idea how to handle it.

It wasn’t the worst thing in the world. It worked fine. One drawback though – it pollutes the console during tests.

Lesson learned: use logger as a configurable variable (dependency injection, if you prefer). In tests, inject RSpec double and test against received messages.

Only class methods

This cluttered the code and obscured the intention. The code had nothing to do with metaprogramming or anything clever, it was a simple call&forget case. I’ve overcomplicated it.

Lesson learned: avoid class methods in simple cases.

All methods public

All 5 methods were public while only one was supposed to be called publicly.

I remember writing the code in TDD and this is where it got me. I tested only against that one method so presumably, I skipped some “refactor” phase.

Lesson learned: use one class method as a shortcut for creating an instance. Other than that use instance methods.

No encapsulation

The code was a rails runner script. It contained this dirty, Perl-like (or whatever this ugliness is) section:

if $0 == __FILE__
  # run main method
end

Again, TDD got me there. On retrospective, I started the TDD session with a wrong question. I asked myself: “what should my script do?“. I should have asked instead: “what should my code do?“, and then plug the console interface to such unopinionated code (D from SOLID, if you’re curious).

Lesson learned: encapsulate as much as possible into a Service Object. Make the script itself as straightforward as possible. It should only call the Service Object after unpacking the parameters from I/O.

Poor specs structure

As I mentioned, the code had a decent test coverage (thanks to TDD). Even my client boasted me – he could easily understand what was going on.

However, there was a dozen of its in a single describe, without nesting or anything. Each case was also lengthy and written similar to Arrange Act Assert fashion.

This is not how I write specs today and there’s a good reason for that. Returning to the code after some time caused me to read all the specs. It was clearly the fault of the structure of the specs.

Lesson learned: boldly use context, before, and let to ease incidental diving into the code and specs.