1. 0
  1. 65

    Absolutely shit title.

    The content is “Bad code reviews are bad. Good code reviews are good.” Not really worth reading.

    1. 12

      Saved my time. Almost clicked.

      1. 3

        Dangerous to see titles like that while drinking morning coffee. I about bought a new monitor over a title that was worth spitting on. It had to be what you said or bullshit given weight of empirical evidence. Moving on…

        1. 4

          Welcome to Medium tech publications.

        2. 12

          Beginning to really, really dislike this kind of attempted attention grab. I’d have respected the article more had the title been “Toxic Code Review” or something.

          1. 1

            I’ve just banned everything containing meme titles. „considered harmful“, „no silver bullet“, „for profit and fun“.

          2. 5

            This article is rehashing a common misconception of immediate and future work. Teaching folks to assess and assign those accurately is important, especially for junior devs needing to make progress at the hands of the pedantry of peers and more experienced reviewers. Warning: suggestions are Github-workflow specific.

            Arguing over formatting. Things like tabs vs spaces and brace placement same line or next line should be settled and enforced by the build.

            Integrate a style checker with the project’s build system or CI system. ScalaStyle, scalafmt, Rubocop, CheckStyle, gofmt, whatever. Agree on a style and set it as law with automatic conformance. Argue over //noformat blocks or something if you really want to get into it (and you should not).

            Reviewers competing for the most clever one liners or ever more layers of abstraction and indirection. Code should be maintainable and the appropriate level of abstraction. To me this means the minimum level of abstraction. This is obviously a contentious point. Talk it out!

            There’s a difference between an immediate improvement and future improvement. Immediate improvements should block a merge. Reducing something to a one-liner when it doesn’t have a performance benefit is not an immediate improvement. It may be more idiomatic but it is a future improvement: submit another PR if you’re so adamant about idiomaticity but don’t force me to waste my time with it.

            Arguing over variable, method, class, or package names. Decide on a methodology and stick with it.

            Some style checkers handle this (no single- or double-letter variables, no keyboard mashing, etc.) but this could be a fight worth fighting sometimes. Naming is hard. Again, submit a PR to the branch if you’re so adamant. Show me the code!

            Reviewers requiring weird things like the Assert.assertTrue example above. Ideally all standards would derive from industry standard norms, like SOLID principles. If a reviewer can’t express something in those terms then it shouldn’t be a coding standard.

            See my sentiments about idiomaticity. Submit another PR if you’re so adamant about idiomaticity but don’t force me to waste my time with it.

            Previously unstated requirements are presented as code review required changes. Missed requirements ought to be an enhancement for the next sprint.

            Future improvements. New PRs later in the development cycle so that the unstated requirements can be captured as a separate unit of work.

            Reviewers requiring changes in code that’s not part of the review. Code beautification and refactoring ought to be tasks in the next sprint as well.

            WTF people do this? That’s future work: file another issue, cite the PR as the source (“discovered while reviewing #256”), and move on with life. Maybe submit a PR and reduce that mean-time-before-fix number.

            1. 4

              I think it is useful to make the original author do idiomatic code improvements. Otherwise the person will be more likely to continue to write non-idiomatic code. Especially if the code being reviewed was written by a more junior member of the team and/or someone new to the language. So my opinion is to do it during code review OR make them do the follow up PR.

              Besides that I agree with you.

              1. 1

                Yeah, it’s not particularly useful to allow a junior to continue to “write PHP in Ruby” or whathaveyou indefinitely, and it should block them merging in my opinion. Otherwise the “temporary” non-idiomatic code will likely be as permanent as every other “temporary” hack.

                “Merge everything but the most egregious problems and clean it up later in other PRs” is a recipe for rapidly eroding the quality of your codebase.

                1. 2

                  By the time “PHP in Ruby” or the like makes it as far as a PR, it’s usually too late, in my experience, and a sign that some kind of necessary guidance is not being provided earlier in the process. I haven’t seen these kinds of problems fixed in the PR stage (unless they are very minor things).

                  What I’ve seen happen is that the reviewer is dismayed by the code and points out a few specific solecisms because it’s often infeasible to explain language idioms in depth in a PR comment. The junior developer is annoyed at what seems like nit-picking and will likely fix the specific points mentioned in a way that doesn’t really address the overall non-idiomatic-ness of the code (because they don’t see the larger patterns they are failing to follow, since if they did they would have written more idiomatic code in the first place). In the end, nobody is happy and the code is not much improved.

            2. 2

              Ideally all standards would derive from industry standard norms, like SOLID principles. If a reviewer can’t express something in those terms then it shouldn’t be a coding standard.

              Meh, if our industry made use of scientific practices in determining norms that would be good advice. Otherwise “industry norms” are just fashion.

              Just one example: I’d love to see some scientific research on the impact of column length restrictions in code reviews. Here are some of the issues we’d need to investigate just to start to get a feel for the problem area.

              • A measure for code comprehensibility
              • Code comprehensibility at different column widths
              • A measure for diff comprehensibility (bugs caught in code reviews)
              • Comparison of side by side vs inline diff techniques in terms of diff comprehensibility
              • Survey of current resolutions (assuming we want to display X characters on the screen, what % of developers can do that)

              If you can use that research to back up your argument that “80 columns is a good standard because it allows code review tools to do side-by-side diff that fits on your monitor without wrapping” then I’ll listen to you.

              Otherwise you might as well be telling me your favorite taco spot. In fact that’s a good analogy for the unscientific team standards today. Everyone has to go to lunch together and eat at the same restaurant so you try and pick something that tastes good.

              1. 3

                I don’t need science to say that N columns at a certain font size at a certain distance is readable in a side-by-side display. Depending on how good your eyes are and how clear your display is, N could reasonably be 79. Or perhaps 99.