software engineer, conference speaker, caffeine enthusiast ;) Co-founder of Poznan Scala User Group
A few weeks ago a colleague asked me about a piece of code I had written.
Unfortunately I couldn’t answer any of his questions.
The code was available, it was something we could call “self-documenting code”, there were meaningful function and variable names, I did not use any clever tricks, there were tests. I think the assertions and test names were readable and could be used to figure out what I was trying to accomplish by writing the code.
But it was not enough.
The problematic code was an actor (if you do not know what it is, read about actor systems).
Let me explain why it was problematic.
I used the word “synchronous” in the actor name
Actors are synchronous. An actor never performs two actions at the same time. You can create more instance of the same actor or use the actor to start processing something in a different thread, but one actor is doing only one thing at a time. So why did I call it “synchronous”?
the actor was creating a JSON object and sending a HTTP request, nothing fancy
Why did I need a separate actor implementation for something like that? What was so special about it?
it was using a non-blocking HTTP client
It did not need to wait for the response. The actor could start sending another request even if the previous had not been completed yet.
the behavior of the actor depended on previously processed messages
In Akka (and probably other actor systems) there are become/unbecome methods which allows you to change the implementation of the actor depending on a received message.
My actor behaves in two different ways:
There was nothing explaining what was the reason of it. No comments explaining why I wrote the code. It was not mentioned in the documentation.
When the colleague asked me about it, I could not explain it, because I had already forgotten everything.
It seemed I had seriously over-engineered the code for no reason. I was trying to prevent actor from sending too many request to the server at the same time. In this case “too many” means more than one. That was the obvious part.
Why the server could not handle more messages?
Was there a real reason or was I too careful?
I have no idea.
We could assume that there is a valid reason for keeping the code and do not touch it. We could be afraid to remove it, because “something may brake and it is going to be our fault”. Fortunately, we know we can restore the previous version of the service with a single click of a button.
There was no reason to keep the code, nobody knew why it is there, hence we removed it and… nothing happened.
I am absolutely sure there was a reason to write that code when I was writing it. Now I know that we no longer need it. Or… maybe we do? Maybe one day something is going to fail because of that change.
If it happens, we are going to restore the old solution and document it properly. For now, there is no reason to keep it and waste our time every time we read unnecessary code.
It should not happen, I should not need to experiment with the software to check why the code exists. Decisions should be documented. Can we just start writing comments?
The comments should not explain what is your code doing or how it is doing it. You have the code and its tests for that.
What I need in comments is an explanation why I should not remove that code. If you have made a decision that affects the architecture of the application, please tell me about it in the comments. If you have tried a few different approaches before writing the code, tell me about that. So I do not waste time thinking “maybe we should have done it differently”.
I hope I will remember about it…
This text has been cross-posted from: mikulskibartosz.name