Why I requested to change the whole logic of code in code review.
A lot of articles say what Code Review should be like or shouldn’t be like.
- You should not criticize the code.
- You should encourage commiters to work with better code, not discourage them by giving negative feedbacks.
- You should check(test) it works well or on the contrary just check the readability and naming parts.
- You should not bring up the structuring and logic, it should be discussed beforehand, versus should check structuring and the logic.
Sometimes it feels like easier said than done and get lost in what is wrong or right.
Recently I suggested a coworker to change the logic of the code on the review. I wouldn’t explain what issue it was about, but would say it alarmed me in 2 aspects.
First of all, it was way too complex.
The feature was simple, dispatching the action on time.
But It took me more than 7 minutes to take what was happening there and there were new custom operators which also need time to digest.
Secondly the data stream was a bit off that it was declared in the series processing, but they were more likely in parallel relationship, and the same logic showed 2 times in the code. I could see the neater way to fix the part and that was the time I was thinking whether I should send change request or not.
Honestly I hesitated a day before asking, because I know changing logic in the code review is awful, have to check all the other side effects and test the parts again. Especially when the code does its job, taking the risk of something going wrong is kind of stupid and that thought kept me from requesting.
But then, I changed my mind the next day.
After Daily meeting, I asked him to talk in private and delivered my opinion.
As I expected he wasn’t thrilled with my request and told me that he does not considered code itself good/bad, right/wrong as long as it does its job.
He wasn’t sure that fixing was worth of his time since the feature is not main feature of the application. Unfortunately I disagreed with his opinion, not being main feature and not requiring more than half a hour to fix were the one of reasons to changes and got me thinking how I could talk him into what I’d explain here.
Affordance is the term used in the UX design that a feature or properties of an object leads the user to afford an action or concept in some way.
For instance, when you see doorknob you know how to react on it(pull or push or other way), and you see chair you know where to sit.
I think this concept is not limited to design field only, it also applies to the code we write. we are not computers, we don’t check all the characters one by one, instead, we assume and conceive, and sometimes even imagine the function.
In a team, you don’t write code alone. You write code with coworkers and even if you have the whole ownership of the code, the chances are, that you might not manage that code for good.
Have you ever experienced that you read 200 lines and then got to realize that the part was for only sorting? That is real frustrating. Mostly it is not because the code uses difficult method or difficult algorithm, it is because you can’t follow the code lines.
A developers who gets to face long and complex code naturally thinks that the part would be complex and hard and try to prepare him or herself for it, and it can be a hinderance to figure out what it actually is doing.
So writing code unnecessary long and complex will waste your coworkers’ time and yours in the future. With good structuring, it is easy to see the main logics and grasp what they are doing.
For me, good code is the code saves you and your coworkers of time. If your one hour saves each 4 of them 10 minutes, you might say it’s not a benefit, but you or your coworkers might have to read that part several times in the future.
Developers are human. They also avoid to put their hands on the code looks way too complex, unless it causes an urgent bug, that code might lose the chance to get better ever.
So I talked him into change, I can’t tell it was because he was convinced or I have seniority over him, but that code lines cut in half, and there were no repeated logic, and I can say that it was simpler and easier to understand.
Of course I don’t say that every code should be simple and precise. Some code meant to be complex by what it does, sometimes we don’t have time to make it precise. Also the part I referred was simple small-scoped job, and it wouldn’t take long to fix it.
I want you to keep this one question when you get this kind of code review.
Are you sure your code does not waste time of others with no good reason?
If you are sure, that’s great.
But if you have even small doubt about it, please rethink your reviewers request.