Problems with code reviews
Cross-post from a personal one (http://www.eldar.com/node/200) as usual ...
Yes, yes, I know ... It is very unusual to swear on code reviews (code revisions), especially in the world where they are perceived almost as the eleventh commandment, for disrespect for which it is easy to please at the fire ... So, bear with a little heresy, I will explain everything!
So ... I am not saying that code revision is bad. Just everything in our sinful world has its advantages and disadvantages. Or, as the Romans were tired of the wisdom of the Greeks - cons et pros. So, I would like to draw your attention to some con code revision, which is usually not mentioned out loud ...
Imagine you are working in a small team and you are terribly busy creating a new product. Please note: "terribly busy." And - surprising, is not it? - as in any other product, you have bugs. And a bug is such a thing that needs to be fixed. No fools, no kidding ....
')
Now, the question on the backfill. How will you fix the bug? As far as I know, there are only two philosophies how to do it: patches and refactoring. Well, yes, yes, there is still an idiotic "just fix it", but we will not stoop so low, right? Idiots who do not understand what I am talking about can stroll and not get into our conversations. And for us - the rest - the choice is still there. So patches or refactoring?
So, what do you think is the right way to fix bugs? Yes, yes, there are cases where the patch - this is the right decision. For example, Quick Fix Engineering - when you need to deliver a patch to a critical user with several thousand copies of your software. Another example is when a product has long been made, and all you want is to touch it as little as possible. Well, and finally, situations where every fix is ​​a very big risk. Say, when you fix software for a spacecraft in orbit of Jupiter ... real time ... and any bug will leave your customers with a piece of dead iron, which costs many, many millions of dollars in ... the very same orbit of Jupiter.
However, in most cases, I would argue that in terms of “good engineering”, refactoring usually outweighs the patches. Well ... not only surpasses ... but how to say this ... "like a bull to a sheep"! Not true. Let me explain with an example.
Given the short size of the article, I will have to give a rather primitive example, but still, a very vivid, I think. So, imagine that you have a method of LoadingFinished (), which is called at the moment when the loading of a piece of media is finished, and which means that you can start downloading the next piece. And then you noticed that one of your colleagues inserted some completely idiotic code into it. No, let's be fair, the code would be completely non-idiotic if it were inserted not into DownloadFinish (), but into DownloadFinishedFix (). Well, what to do, unlucky identifier. But as a result, you have a choice of two options:
1. Just transfer the code from DownloadFinished () to ZagurzkaCompletedFaul (). The bug will be fixed. The fix takes a few lines ported slightly down in the file.
2. Make (1) and rename Download Complete () to Allow Download of the Next File (), which, in fact, this function is, thereby preventing similar errors once and for all. The fix will affect a dozen or two files.
Now, don't forget, your team is really busy. What are your chances of getting a code review quickly, if you select (1) or if you select (2)? Yeah. Well yes. For sure. 1. When you try (2), the guys will quickly take a look at the list of changed files and immediately precipitate. And for a good reason. And you are absolutely not in a rush to keep the damn-those-code on your car and pull with the check-in, right? Exactly.
And what is the result? The result is that your product will get a patch, not a refactoring. And a week later, another, someone else plunges into this very same LoadingFinished (), and you will rule the next twin bug ... Well, if it's the same, or worse, then ...
I don't know, honestly, what does it look like? Am I really talking about something serious, or do I think?
Yes, yes, I know ... It is very unusual to swear on code reviews (code revisions), especially in the world where they are perceived almost as the eleventh commandment, for disrespect for which it is easy to please at the fire ... So, bear with a little heresy, I will explain everything!
So ... I am not saying that code revision is bad. Just everything in our sinful world has its advantages and disadvantages. Or, as the Romans were tired of the wisdom of the Greeks - cons et pros. So, I would like to draw your attention to some con code revision, which is usually not mentioned out loud ...
Imagine you are working in a small team and you are terribly busy creating a new product. Please note: "terribly busy." And - surprising, is not it? - as in any other product, you have bugs. And a bug is such a thing that needs to be fixed. No fools, no kidding ....
')
Now, the question on the backfill. How will you fix the bug? As far as I know, there are only two philosophies how to do it: patches and refactoring. Well, yes, yes, there is still an idiotic "just fix it", but we will not stoop so low, right? Idiots who do not understand what I am talking about can stroll and not get into our conversations. And for us - the rest - the choice is still there. So patches or refactoring?
So, what do you think is the right way to fix bugs? Yes, yes, there are cases where the patch - this is the right decision. For example, Quick Fix Engineering - when you need to deliver a patch to a critical user with several thousand copies of your software. Another example is when a product has long been made, and all you want is to touch it as little as possible. Well, and finally, situations where every fix is ​​a very big risk. Say, when you fix software for a spacecraft in orbit of Jupiter ... real time ... and any bug will leave your customers with a piece of dead iron, which costs many, many millions of dollars in ... the very same orbit of Jupiter.
However, in most cases, I would argue that in terms of “good engineering”, refactoring usually outweighs the patches. Well ... not only surpasses ... but how to say this ... "like a bull to a sheep"! Not true. Let me explain with an example.
Given the short size of the article, I will have to give a rather primitive example, but still, a very vivid, I think. So, imagine that you have a method of LoadingFinished (), which is called at the moment when the loading of a piece of media is finished, and which means that you can start downloading the next piece. And then you noticed that one of your colleagues inserted some completely idiotic code into it. No, let's be fair, the code would be completely non-idiotic if it were inserted not into DownloadFinish (), but into DownloadFinishedFix (). Well, what to do, unlucky identifier. But as a result, you have a choice of two options:
1. Just transfer the code from DownloadFinished () to ZagurzkaCompletedFaul (). The bug will be fixed. The fix takes a few lines ported slightly down in the file.
2. Make (1) and rename Download Complete () to Allow Download of the Next File (), which, in fact, this function is, thereby preventing similar errors once and for all. The fix will affect a dozen or two files.
Now, don't forget, your team is really busy. What are your chances of getting a code review quickly, if you select (1) or if you select (2)? Yeah. Well yes. For sure. 1. When you try (2), the guys will quickly take a look at the list of changed files and immediately precipitate. And for a good reason. And you are absolutely not in a rush to keep the damn-those-code on your car and pull with the check-in, right? Exactly.
And what is the result? The result is that your product will get a patch, not a refactoring. And a week later, another, someone else plunges into this very same LoadingFinished (), and you will rule the next twin bug ... Well, if it's the same, or worse, then ...
I don't know, honestly, what does it look like? Am I really talking about something serious, or do I think?
Source: https://habr.com/ru/post/37837/
All Articles