Timeline for What is the purpose of a Code Review
Current License: CC BY-SA 3.0
13 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Sep 12, 2014 at 14:09 | comment | added | SoylentGray | @DocBrown - I do not read this as saying that if you find a bug or problem dont say anything, rather that the focus of the review should not be finding bugs. | |
| Sep 12, 2014 at 7:11 | comment | added | Pimgd | @pacifist your specific example would get totally blasted in a code review. It would also be a red flag for any static code analyzer (Cyclomatic complexity 17!). A code review would quickly identify this function as a problem in semantic style (or even the algorithm!). However. This kind of issue is not just a "style" issue. If you treat it as such then you'll have some really nasty code in your repository real soon; it's just "style", after all. | |
| Sep 12, 2014 at 7:00 | comment | added | Pimgd | @pacifist You're still stuck on the different kind of "style". There's syntactical style and semantic style. Syntactical style is the position of braces and such. Semantic style is how you translate a solution into code; such as the use of a large amount of if-statements. A Code Review is not for determining issues in syntactical style. It is intended for determining semantic style. Both these points are present in the answer: you'll find semantic style matches DO point 3. You'll find syntactical style matches DONT point 2. | |
| Sep 12, 2014 at 0:57 | comment | added | pacifist | @Pimgd it's disingenuous to claim if statements are not a style problem, just a performance one. In my case the cascading if statements will have actually been faster (quick-fail-early) but the readability is horrible since the reader has to mentally cache parts as they scroll through. Damien's PBP (the perl style guide) is quite clear about 'Avoid cascading if'. Heck try and read the code here beenishkhan.net/2012/05/27/… ... and yes I'm talking about Perl. I can hear the chatter of keyboards starting jokes already. | |
| Sep 11, 2014 at 22:52 | comment | added | Maja Piechotka | In addition to @DocBrown there are cases which cannot be easily testable - data races, some types of deadlocks, livelocks, undefined behaviours/values (mostly in C/C++ but the order of elements in hash tables in undefined as well) or resource leek (opening file in a loop might be a bad idea even with GC). Some of those things can be detected by sufficiently smart -compiler- static analysis. | |
| Sep 11, 2014 at 17:49 | comment | added | Russell Borogove | +1 for knowledge transfer; IMO this is the greatest benefit. | |
| Sep 11, 2014 at 17:34 | comment | added | nobody | I think code reviews have more of a QA role than this answer admits. Yes, tests should catch everything but they don't always - I've seen too many tests that can't fail because they're checking the wrong thing. Until things like coverage tools get much smarter, having a fresh set of eyes inspect code and tests before you ship is a good idea. | |
| Sep 11, 2014 at 16:49 | comment | added | Nope | @Pimgd: In addition, when following SOLID I would imagine to many if statements could possibly mean a class has to many responsibilities and/or abstraction might be needed to deal with that. Though I'm not sure if that logic applies across all languages evenly or to some at all. | |
| Sep 11, 2014 at 9:23 | comment | added | Doc Brown | When doing a review, I often spot things where I think "this looks like a bug", then I write a specific test case to prove it is a bug. So one of the many goals of code reviews is finding bugs. That's why I think the viewpoint "either code review or tests" is a little bit too single-edged. | |
| Sep 11, 2014 at 8:47 | comment | added | Pimgd | @pacifist That's not the kind of style the answerer is describing. Style is about locations of braces, indentation and so on. If your junior developer is using too many if statements you have an entirely different problem from style; a general attribute of coding STYLE is that it doesn't impact performance. And I think that a significant amount of if-statements will impact performance. | |
| Sep 11, 2014 at 8:25 | comment | added | Gusdor | I have several colleagues who frequently cite significant numbers of functional defects being located in code reviews. At very least it is a happy side effect. | |
| Sep 11, 2014 at 1:22 | comment | added | pacifist | your last point on nitpicking style issues I don't entirely agree with - we've just had a harrowing experience reviewing code of a junior developer & the most glaring complaint was actually on style, but not the kinds of style problems that are easily programmatically enforced.... waaaaaay too many if statements for edgecases etc; issues that yes you could make a computer find in some cases, but most were not issues worth generically finding by script. Takes 30 seconds of reading for us to start seeing it, another 30 to explain to the dev & hopefully ammend the issue. Still in shock at it :/ | |
| Sep 10, 2014 at 20:09 | history | answered | amon | CC BY-SA 3.0 |