Hi,
The more I am working at Software production area (and it is almost 20 years), the more I am coming to the conclusion, that support and maintenance in many cases are more essential then “beautiful” code with using design patterns. Don’t think that I have something against best practices – quite reverse. I am fond of design programming patterns and I am using it constantly by myself, the same as I am promoting other developers to do it. But, unfortunately, from what I see, programmers are often consider design patterns to be a golden bullet. Moreover, software developers are also often strive to the perfect code. But should it be really perfect? From my point of view, the correct answer here is – IT DEPENDS
Let me present you what I really mean using example from real world that has happened indeed. Very good and experienced developer, let call him Jack, got a rather simple task – create a command, that will be running once per day and gather opinions about the company (he is working at) from a popular world’s rating system. To preserve things to be abstract, let’s call that external system as “X”. Looks to be an easy task, don’t it? But Jack, as a responsible guy, who always likes to make things to be done perfect created next solution – below I will provide small simplified diagram that will help to understand main realization idea:
At final pull request 15 files appeared (I even missed some of them at diagram to not complicate the situation even more), final solution had over 500 lines of code, and yes – it took me 30 min, having a fresh head, to read all and understand how logic works. Now, let’s open short discussion. First answer that I want to ask is the next one:
Is current code is written properly and with using best programming practices?
Two other good developers gave their approves. I know them, they are good specialists, so probably all is OK. From first glance we are having here a “perfect code”:
- we can mark all check boxes around SOLID, DRY principles
- we see here a data transfer object DTO), which is from many book’s definitions has to be used in case transferring data between different systems or processes. Looks like it is a good place to use it.
- we have here a Mapper design pattern, that performs data transformation
- good level of abstraction
Suppose I could continue to list different advantages of that code. BUT, let’s return to reality. From my point of view that code is BAD. You may say:
Let me explain you why. There are several questions, what good developer should ask himself before starting to provide any coding:
- Is it crucial functionality for the company?
- Do we expect further extending of current function?
- Do we expect often changes at code we are going to write?
At that case it was a minor functionality. Even if it were not working – there would be no dramatic effects at company or business. The API of rating service X is more then stable – it was not changed over the years. There were no plans to extend current function at closest future. So, I disapproved current pull request explaining that, from my point of view:
- it is over complicated solution for such a simple function, that is not going to be modified ever;
- DTO, DTOCollection and Mapper design programming patterns in that current case has no sense, as external API is more then stable, and in fact we are simply “pouring from the empty into the void” as our Review ORM entity has the same structure as DTO by itself, and I dont see something has to be changed here at closest years.
As result my proposition was to preserve all logic at one file – DownloadReviewsCommand. Here is some pseudo code that I proposed to use:
......
$httpClient = new HttpClient();
$responseData = json_decode($httpCLient->sentReqest($SERVICE_X_API_URL, $credentials));
foreach ($responseData as $review) {
$reviewEntity = new RevieOrmEntity();
$reviewEntity->setReviewMessage($review['message']);
$reviewEntity->setRating($review['rating']);
//save entity to db using packages
}
.......It is only the main idea – but anyway – the whole PHP code took some 50 lines including catching exceptions and logging. Next day, we had a hot discussion with Jack around his pull request. He did not agree with me and insisted that it is really good code, and that better to leave it. The main his arguments were something like that – OK, maybe it is over complicated, but it applies best practices, it is working and already tested, as result there is no sense to change it. Sounds reasonable, don’t it? From another side – it is painful to throw into the garbage 3 days of the work, especially when all could be done in half a day. It is really hard to accept such a things – finally I agreed with Jack and gave my approve to the pull request. It was a wrong decision. The most interesting part of that story is ahead.
Several months has passed and Jack decided to change the company as he got a very good offer. Meanwhile, another senior backend developer from his team took the vocation. And as it happens at real life, Murphy’s law took it place. At logs appeared information that review command is not working. Moreover, locally and at staging all worked – but not at production. Despite that there is a golden rule to not debug your code at production, I doubt that we will find at least some software engineer with over 10 year’s experience who had not do it 🙂 . Here is the chronology of events that had appeared. The junior developer, that got the task to fix the issue, at first simply could not understand what is going on with all that design patterns. After spending on it 2 hours, he finally decided to ask a help from more experienced backend developer from another team. Together they spent 1 hour more to understand the whole logic at 100% and estimate all risks related with changing according dependencies for debugging at production. Luckily, the existing DTO and other review related services were used only at DownloadReviewsCommand by itself. As at dev environment all worked, they applied to DevOps, who had direct access to the servers with ask to help them with debugging. Together (3 persons), they spent 1 hour more to debug the issue. Let’s provide some small calculations and try to estimate companies expenditures taking into consideration some average developer’s hour rates:
junior developer 4 man hours x 10$ per/hour + senior developer 2 man hours x 30$ per/hour + devops 1 man hour X 35$ per/hour = 135$ Generally speaking, we have to double that, as without spending time at fixing the error, which is not bringing any profits to the company, developers could spend their time at something really useful. But it is not the end of the story. In 6 months after the incident, business took a decision to remove according functionality completely – as result, all code was removed to the garbage. Now, let imagine that a simplified solution would be applied in the beginning. First of all Jack, would not spend 3 days, but half a day for writing the code. At second, debugging the incident with reviews would not take more then 2 hours (~ 1 hour for junior + 15 min for devops). At 3d, cleaning the code would take 5 min, not 1 hour 🙂
What is the summary:
- Try to keep things simple and do not over complicate code, when it is not necessary. Thing deeper about functionality and it’s further development from business side before starting the coding.
- While writing the code remember about it’s support and maintenance.
- Do not use design patterns only because it is beautiful. As example, DTO, as for me, is good when you need to aggregate results from different sources or when you need to customize essentially the final model instance, or when you expect constant changes at external API. At current case using DTO was excessive.
From my point of view, good developer – that is the not one who knows all design patterns and use it everywhere. That is a person, who can find the most optimal solution taking into consideration business, maintenance and code requirements at concrete existing company environment. Thank you for you attention.
