Pride & Punishment

Hi there,

It's Friday and we all get relaxed at work before officially starting the weekend. No releases or any changes that could potentially cause any bothering or major problems. We're all euphoric and you can nearly feel yourself sat in a terrace bar having a fresh (spanish) lager with some tapas, enjoying the enviable weather of Majorca (my apologies if u live in rainy, cloudy, northern European country xD, I'm being mean now). 

A teammate is about to commit his last change and prepare a Pull Request for the next release, and, finally, off he goes:

Notice the name of the PR. I can assure you that the guys of the company next to us could hear us laugh out loud for a while. 

Let me put you in context regarding this PR and the project to which is related.

The PR code belongs to a relatively young project (almost 2 years old) that was born, in my honest opinion, with several mistakes. First and foremost, no unit tests at all, unbelievably, nowadays, how can a company start a green field project without test coverage!? Money and time wasted and a very promising technical debt on its way. It's not up to our manager or boss to decide whether to do tests or not, it is our decision, since it's part of the development process. It is irresponsible not to do so. At least we have some functional tests for the main functionalities.

Secondly, a manifest abuse of services, those are, Singleton instances of Managers, Parsers, Handlers, Validators, which were created one per each Request and Response and per every kind of Operation to perform: 2 classes (Request + Response) x 8 Operation types = 16 Parser services, 16 Validator services, 16 Managers, and so forth...

Everything is inside a service, no domain logic at all. IoC (Inversión of control, aka Dependency Injection Pattern) via constructor to inject several services to each service, what means endless lists of parameters for each service constructor since one service depends on many others.

Third point, proliferation of logic-less objects just to copy data from ones to others along with poorly chosen names. As a consequence of a bad design of the problem abstraction, we also find the worst code smell stinking from the very beginning: code duplication. Evil !

In conclusion, the project is a real tar pit, and it's a complete shame. Fixing the early design mistakes now is incredibly expensive, it doesn't matter the angle you look, money, time, frustration ...

Note:  For the record, this is not to blame anybody, because there was a day when it was myself who did something similar, who hasn't ? Thanks to that we now have the experience. :) The point here is to learn and help others not to fail as much as you can.

Getting back to the main point of the post, the image with the "awesome", since I've been assigned as a reviewer, I've gone very carefully through the code of my teammate.

I noticed a duplicated method in two different services, I suggested to add the dependency of the first to the second via constructor, so the second could invoke the method using the first. My team mate argued that he preferred to duplicate the code rather than creating an extra dependency, which, at the end of the day, didn't have too much sense. Eventually, we decided that code duplication was worse than adding another dependency to the long list of parameters in the constructor.

Besides, we split the method in two, so each one did only one thing,  and we renamed the methods carefully to describe what they were actually doing. All these good practises are described here: Code Clean, [Robert Martin] I strongly recommend the reading.

My teammate, who has a very particular sense of humour, described this situation as: "What is the worst scenario, either drink shit or eat shit ?" We all ended up laughing again loudly in the office.

Have a nice weekend