Refactoring legacy code: Is it worth doing?

I am talking about code that is already working in production, but doesn’t have any tests.

The answers is: Of course it’s worth it. What kind of question is this?!

Well it sounds like a stupid question, but in practice, the answer most developers choose here is NO, it’s not worth it. They just don’t want to touch a house of cards. Sometimes because they are not behaving like a professionals and sometimes because it’s just too hard to change anything, because tests are missing. If they change something, everything will break.

So let’s answer why it’s worth it.

If code is in production, there will be a reason to change. It can be fixing a bug or a new feature, but it will definitely change.

Let’s consider this. Our boss, CTO, Team Lead, or even worse Project “Manager” comes to us and asks us to make this new feature that our client wants, and it needs to be done by the end of the day. So if we are lucky and we are practicing some form of Scrum, we can say fuck off (in a nice way) too him/her and wait for the next iteration. But as I figured, most of the developers are not that lucky.

So what to do?

You have an option to change the code in 10 places and it will be done by 5PM Today.

What are the chances you just broke something?

Big, too big. There are no tests, change had to be done in 10 places and you are going to see side-effects. It happened to me a hundred times. The result is that you are shipping product that no longer works as intended and the client will lose trust in you and if this repeats, you will lose a client.

The correct answer here is that you don’t make changes in 10 places in your code. Never change or add new code to the code that is not tested.

Then how to implement this new feature?

There is two methods that we can use to change existing, non-tested, legacy code.

Sprout method Wrap method

When you need a new feature implemented, you don’t change existing code, you add that feature in a new method/function and then call that method from place where new functionality needs to be. This is called sprout method. This is good approach because you are clearly separating old from new code and by doing that you are making smaller units of code that can easily be tested. If you put new functionality into the old code, it’s very hard to write a test for that, but by doing this, you can write test for the new code only.

Let’s see example of changing old code. So we have function that displays players name and their score.

public function showPlayersHistory() { $players = $this->getAllPlayers(); foreach ($players as $player) { echo "{$player->name}: {player->score}"; } }

Now client wants to have only players that are registered to their new premium service, displayed here.

So what we would do is add condition inside foreach loop.

public function showPlayersHistory() { $players = $this->getAllPlayers(); foreach ($players as $player) { if ($player->isPremiumMember()) { echo "{$player->name}: {player->score}"; } } }

Even though this seems like a simple change, this is wrong approach to take. We are just cluttering old code more, consider that it will probably grow more. What if clients want to display only players that have payed membership, are over 18 years old and have scores more then 100. The example is trivial, but you get the bigger picture.

So let’s add a new feature by using sprout method.

public function showPlayersHistory() { $players = $this->getAllPlayers(); $payedMembershipPlayers = $this->getPremiumMembershipPlayers($players); foreach ($payedMembershipPlayers as $player) { echo "{$player->name}: {player->score}"; } } private function getPremiumMembershipPlayers($players) { $payedMembershipPlayers = []; foreach ($players as $player) { if ($player->isPremiumMember()) { $payedMembershipPlayers[] = $player; } } return $payedMembershipPlayers; }

So what we did here is we injected a function call to the old method, and we extracted all logic into new one. This new one, we can now test. It’s quite easy to test it actually.

Sometimes this is not the right way to add behavior, because we are breaking a Single-Responsibility principle in our method. The method was intended to only show history of players, and now it knows about premium membership. In that cases we use Wrap Method

Let’s see an example. We are gonna stick to our little game. We have a service in place that saves to the database when ever the high score is broken.

public function checkHighScore($player) { if ($player->score > $this->getHighScore()) { $this->saveHighScore($player); } }

Now our client wants to send an email to the old high score holder informing him that he lost his first place. So we do this in old-fashioned way.

public function checkHighScore($player) { if ($player->score > $this->getHighScore()) { $this->saveHighScore($player); if ($this->getHighScoreOldPlayer()->username !== $player->username) { $this->sendEmail($this->getHightScoreOldPlayer()->email); } } }

We are again changing the old method, and we are not sure if this is going to break something.

Let’s try to add this feature with a wrap method.

public function checkHighScore($player) { $this->saveHighScoreToDB($player); $this->notifyOldHighScorePlayer($player): } private function saveHighScoreToDB($player) { if ($player->score > $this->getHighScore()) { $this->saveHighScore($player); } } private function notifyOldHighScorePlayer($player) { if ($this->getHighScoreOldPlayer()->username !== $player->username) { $this->sendEmail($this->getHightScoreOldPlayer()->email); } }

So what we did here was we made a new function that was named as the old one, and the old code we transported to the new method. Also we extracted all new features to a brand new function. So now we have three functions. But the two new ones are private, and for the client that is using this class, nothing is changed. He is still calling checkHighScore method. But now we have units that can be tested, without changing the logic of old code. We just reorganized it in a new way. If you are using IDE, renaming can be done automatically. If you are not using IDE, you really should start.

The difference between Sprout Method and Wrap Method is pretty trivial. You are should use sprout method when you choose to write a new method and call it from an existing method. You should use Wrap method when you choose to rename a method and replace it with a new one that does the new work and calls the old method.

So to go back to our boss, when he says that feature has to be done by the end of the day, this kind of refactoring while adding a feature is going to lose you some time. Well let me tell you a little secret. No feature is that urgent that it has to be done in a day. If it is, then that company has some other issues.

Also it’s important how to communicate that with you boss, and I am going to write about that in one of my next posts, but before that, you can read chapter 2 What do I tell my manager in [amazon_textlink asin=’0201485672′ text=”Martin Fowler’s book Refactoring: Improving the design of existing code” template=’ProductLink’ store=’housefrommars-20′ marketplace=’US’ link_id=’1b3d7934-7d92-11e7-947b-dddc15e52ded’].

Conclusion is that you should always take your time and do the job right, no matter what is the deadline, because you doing the job now, will save you from doing more work later.