4 ms·

As long as the code does what it is supposed to do with acceptable performance then I don't comment and approve the PR. Because unlike people that think there

As long as the code does what it is supposed to do with acceptable performance then I don't comment and approve the PR. Because unlike people that think there are right ways I know that it's all religious arguments. Last I checked there are 0 credible reproduced scientific studies about what is the correct way to program. Or even a better way. So as far as I'm concerned it is all opinion based and sure I also have my opinions but, consistency, rule of least power, DRY, agile, OOP, single responsibility principle etc etc are all religious notions. There are only 2 things that matter: does it work? does it work fast enough? And normally the arguments boil down to: "Oh but it is not changeable!" - maybe you should make something new "I can't read it!" - I also can't read Chinese "I don't like it" - well not everyone likes chocolate

dwild 8y ago
I agree with you, a code review isn't meant to get the best code ever, mostly because as you said, there's no universal correct way, but it's meant to make sure the code change is doing what it's means to be doing. What is not clear in your comment though is that that means looking at the code and making sure there's no mistake too in it too. It may seems like it's working as expected, but maybe you'll find an edge case that was missed by the original developer. It's possible too that the solution of the original developer was clear to him, or that it's clear knowing the context of the ticket, but once in the wild, that code may become incomprehensible (which later on can bring more error because someone misunderstood why that line was there and changed it incorrectly for example). That can be easily fixed by adding a tiny comment there. Sure doing what it's supposed to do in an acceptable performance is great, but I think there's a bit more into a code review and ignoring theses can be quite problematic. > "I can't read it!" - I also can't read Chinese Doesn't this show there's an issue though in the long term? If you are the only one that can read Chinese in the company, maybe it would be a better idea to consider using English?
strken 8y ago
In practice there are things that are not religious arguments nor problems of technical correctness. For example, in a recent code review I found a function where an argument was being passed in but not used, and a constant of the same type with a similar name was used in the body of the function. That obviously happened because the author got halfway through refactoring their code and didn't realise, and we were both happy to fix it.
zestyping 8y ago
In most cases, designing for maintainability and reducing the likelihood of future bugs is more important than maximizing speed. Or to put it another way, the human costs of maintenance and bugfixing are usually large, and may even be the largest costs on a project.

dwild 8y ago

I agree with you, a code review isn't meant to get the best code ever, mostly because as you said, there's no universal correct way, but it's meant to make sure the code change is doing what it's means to be doing. What is not clear in your comment though is that that means looking at the code and making sure there's no mistake too in it too. It may seems like it's working as expected, but maybe you'll find an edge case that was missed by the original developer. It's possible too that the solution of the original developer was clear to him, or that it's clear knowing the context of the ticket, but once in the wild, that code may become incomprehensible (which later on can bring more error because someone misunderstood why that line was there and changed it incorrectly for example). That can be easily fixed by adding a tiny comment there. Sure doing what it's supposed to do in an acceptable performance is great, but I think there's a bit more into a code review and ignoring theses can be quite problematic. > "I can't read it!" - I also can't read Chinese Doesn't this show there's an issue though in the long term? If you are the only one that can read Chinese in the company, maybe it would be a better idea to consider using English?

strken 8y ago

In practice there are things that are not religious arguments nor problems of technical correctness. For example, in a recent code review I found a function where an argument was being passed in but not used, and a constant of the same type with a similar name was used in the body of the function. That obviously happened because the author got halfway through refactoring their code and didn't realise, and we were both happy to fix it.

zestyping 8y ago

In most cases, designing for maintainability and reducing the likelihood of future bugs is more important than maximizing speed. Or to put it another way, the human costs of maintenance and bugfixing are usually large, and may even be the largest costs on a project.