Monday, December 1, 2014

I'll refactor this later

Imagine this...

You're finishing your current ticket. The project manager keeps pinging you on IM, whether it's finished. This feature is important to a number of users. The business pressure is high. They want you to finish it ASAP.

"Could you deploy it before the lunch time?" - they beg.

You made some shortcuts in the code. It was just faster to put the code in the controller. You know how service objects are better, but this time it was just faster.

"I'll definitely be refactoring this later" - you think.

Just before the lunch, you push to master, run the deployment script. Everything seems to work. At the lunch, you're thinking how you're going to improve the code - extract the service object, add some more tests to cover the if branch that was added just at the last moment.

Back at the office. Everybody knows that you finished the previous ticket and the PM is happy. From the team perspective - you're now available for the next task.

"Could you help me debugging this problem?" - your colleague asks.

"We need the new reports in the admin panel" - the PM is back to you.

There's no way you can now fix the code shortcuts, you've made in the morning. You switched the context 3 times since then. There's just no time to do it. Other important things are waiting for you.

Did you just increase the technical debt? Is it your fault? You hate this kind of situations. You feel guilty.

Is there a way of avoiding such situations?

Can the code be debt-free?

In that story, you didn't manage to fix the code.
How it could have been done better?

Some people would say that you shouldn't deliver, before the code is great. The task is not finished, before the refactoring is finished.

"It's unprofessional" - they say.

There's some truth to that, but it's based on more assumptions.

"It's OK to have some technical debt, it's a pragmatic approach" - other people say. "Just document the problem somewhere" - they add.

Have you been in projects, where a special place for documenting the technical debt was created?

Some add it to the Pivotal/Redmine/wiki. Others try to keep a file in the repo - something like technical_debt.txt.

It can work, but it requires a huge discipline in the team. I used to be a big fan of this approach. After several approaches to it, now I think it's less realistic.

What are other options?

There's this "pragmatic" option to document the problem in the code comment. From what I see in the projects, I'm reviewing this may be the most popular option. I don't like it. I've looked at dozens of such comments, looked at the git repo history - the comments are almost always out of date. They're there and no one ever fixes the problem.

I really like another option. This practice alone isn't enough, but it's a good middle solution.

Document the hack in the git commit message.

Just write about the situation, why it was created like that, what were the time constraints. You may try to justify it a bit. You may explain how it can be fixed in the future.

Those messages are not part of the code, thus they don't make the code less readable. They are meta information, available to everyone, any time they want.

See a bad code? Just use your IDE to show the git logs for this place.

Bad code has other bad consequences. As developers, we like to point out the places, where code is wrong. Sometimes, a hack is implemented in 5 minutes (pragmatic), but then it takes 2 hours in the future for 2 people to discuss why it even got to the repository.

That's the real meaning of a debt. A 5-minute hack can turn into 2-hour discussion.

I've listed several approaches. I've tried them all. None of them is really good, right?

No hope then?

Well, there's another way of thinking. It's probably less spectacular and it requires better skills, but it can reduce most of the problems above.

First of all, there are hacks and hacks.

Some code should never be written in the first place. If you wrote a >100-lines method from scratch then it's a mess, not a code. Such code shouldn't go the repo. It doesn't matter if it's tested or not. There are some basic rules of coding that apply to our projects. It might be good for just spiking - writing some code to see if the concept can work. Afterwards it gets deleted.

Other cases are more on the technical debt side of things.

It's good to have such skills, that it doesn't make a time difference for you to write the code inside the action or in the service object. I just don't believe the time spent on typing those few characters/words more makes a difference. We're talking about seconds here, not even minutes.

More often, you're in situations, where you need to tweak some existing code to make the feature work. The original code wasn't yours - it doesn't even matter. What matters here is that this place requires a cleanup anyway. Let's say that you didn't have time to improve the existing code. There was no time for the Boy Scout rule. You've extended the existing action by some new 'if' statements. The whole path now works.

Now, this is the problematic situation. It's not your fault that the code was so bad at the beginning. You didn't improve it, though. You even made it worse. How much worse? It depends.

It's good to have a set of rules that all team agrees on.

Rails is so great, because it comes with conventions. As a team, you're not only choosing the web framework but also a convention framework. The problem is that at some point The Rails Way is not enough. This is the scary time when the team may not have enough guidelines. This is the time when it's not so clear anymore, where to put code and how to structure it.

That's why we coined the term The Next Way. It's a label to put some practices into. Service objects, form objects, adapters, repo objects. This is our way to solve the problem of missing conventions. It doesn't solve all of the problems, however it does help in limiting the length of the discussions.

The Next Way is just one possible set of conventions. We go with that but your team may choose anything else. What's important here is to have something that the team can agree on.

Thanks to The Next Way, we know what's the best format of a service object. Yes, there's lots of formats and discussing which one is best takes time.

So, The Next Way describes the goal. Your code is in point A and The Next Way code is Z. There's lots of steps in-between.

Being aware that improving code is a process helps a lot. Some would argue that being in point C is still bad. As long you as you all know that it's better than A - you can all agree that it's an improvement anyway.

Let's take an example here.

Imagine that your new feature requires displaying a new sentence in the view which shows the total value of the orders. The current view and action is very @ivar-heavy. There's no service object here, no repository objects as well. If you know The Next Way, then you know that removing @ivars is one of the steps we make to enable us for further steps.

In this interpretation, adding a new @ivar, like @total_order_value makes the code worse. It's not a big deal, but it's definitely not an improvement.

Now, what's good when you all have the commons set of techniques, like The Next Way is that everyone knows that it needs to be fixed. If you didn't have time to do it (like in the story we started with) in the first place - it doesn't really matter that much.


Because it's very easy to fix it. Once you know what is the goal, it's a matter of minutes to just turn the @ivar into an explicit rendering of a view following the "Explicitly Render Views with Locals" recipe.

Whoever next comes to this code and is more lucky with the business pressure can just fix it (with the help of the recipe if needed), commit, push, deploy. That's it. No lengthy discussions, no blaming, all clear.

I've seen many teams struggle when they entered the Beyond-The-Rails-Way phase. This phase is hard to avoid. When it happens, the team has problems with the consistency of the code. Sometimes the code looks like a collection of random blog posts. One technique here, another there. There are some good ideas, but there's no clear goal for all the code.

Once we've embraced The Next Way as a common set of techniques it was all easier. It is like The-Rails-Way-On-Steroids. It's more clear, where the code is. It's now clear when the code is in step C or closer to point Z.

What I'd like to encourage you to with this blogpost is to find your set of techniques in your team. This will help you and it will speed up your team work.

You can take what's in our book as a starting point and fork from there. The "Patterns" part of the book is all about The Next Way and its techniques. This is the description of the goal - the Z points. The "Recipes" chapter, on the other hand is all about the steps between the points.

Buy the book here

If you already bought the book, please consider starting the discussion in your team. Ask if it's clear what are the team conventions once The Rails Way is not enough.

Once you have the book, please don't just read it and put it away. The recipes need to be practiced regularly before they become a habit.

Start today - timebox 15 minutes, choose any action and try to apply the Recipe. I encourage you to start with the "Explicitly Render Views with Locals" one. It's a good start to get the feel of such changes.

There's a second episode of the Rails Refactoring Podcast - we're talking about the DNA of the Rails community and about The Rails Way. The whole episode is 35 minutes. We now also support RSS and iTunes, so subscribe to the podcast if you haven't done so yet :)

1 comment:

Anonymous said...

Good article, just I thunk that it is a developer failure to don't ask for the extra time to fix the fast hack. If PM don't solve the request from the developer then it would be PM failure.

I'm a developer and whenever is needed a rush for a hack I clearly explain it and ask for the time after to fix it.