Timeline for What are common code review processes and what is considered bad?
Current License: CC BY-SA 3.0
9 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Dec 19, 2013 at 22:24 | comment | added | Laurent Bourgault-Roy | I reread your comment and I realize that maybe we have a different outlook due to our methodologie. I work on an Agile Team where there is no lead. Since we are all equal and all responsible for code quality, we must all monitor each others. And code review are done every 3-4 hours before each integration. So cleaning a big pull-request is a few hour if we are very nazi or if we did a refactor that impacted an old and crufty part of the software. Hence why I see code quality comment as a "no biggy". | |
| Dec 19, 2013 at 21:57 | comment | added | Laurent Bourgault-Roy | We'll have to agree to disagree here :) . Technical debt is something that you will have to pay sooner or later (and the more you wait, the more interest you pay). You won't save any penny be delaying cleanup. If you don't take the time to clean it up now, then the next change may cost you double the amount of time because you will have a hard time understanding the code. I work with an 8 years old code base and development had slowed to a grinding halt because of quality issues. We now have an official "internal quality is non-negociable" rule. I can attest that it saved us! | |
| Dec 19, 2013 at 21:38 | comment | added | Dunk | @Laurent:I know what you are saying and in many ways agree. However, that opens a can of worms that tends to snowball. If your company has the funds and schedule to allow for code reviews to take up a significant part of the effort then it's fine (like medical equipment/aircraft projects). But most projects don't have the luxury. Thus, limiting the scope of review comments is very helpful. To offset your concerns, it is the job of the sw lead to oversee developers and their work. They should know who to monitor most closely and get those problems corrected before code review. | |
| Dec 19, 2013 at 21:19 | comment | added | Laurent Bourgault-Roy | One thing however is that if you can't understand the code, or the developer reinvented the wheel instead of using an existing method, or if his method have a cyclomatic complexity of 50, then that is definitely a case for a comment, even if there is no bug. Hard to read code and duplication is a liability, even if its not a bug. That is why I never hesitate to point that a variable is poorly named, or that the solution introduce a temporal coupling that make understanding the code harder. Technical debt must be managed. | |
| Dec 19, 2013 at 20:47 | history | edited | Dunk | CC BY-SA 3.0 |
added 1 characters in body
|
| Dec 19, 2013 at 20:40 | history | edited | Dunk | CC BY-SA 3.0 |
added 1748 characters in body
|
| Dec 19, 2013 at 20:00 | comment | added | user1207047 | Agreed, and you'd hear no argument from me on those grounds. However, the process isn't quite like that. They say it is, and in most cases it appears that only people outside of these three groups are under heavier scrutiny than themselves. They claim others are bad developers but they are the only "developers" on team. | |
| Dec 19, 2013 at 19:56 | history | edited | Dunk | CC BY-SA 3.0 |
added 21 characters in body
|
| Dec 19, 2013 at 19:51 | history | answered | Dunk | CC BY-SA 3.0 |