5 ms·

In the "How it all started section" - > E explained that the main impetus behind the introduction of code review was to force developers to write code that oth

In the "How it all started section" - > E explained that the main impetus behind the introduction of code review was to force developers to write code that other developers could understand; this was deemed important since code must act as a teacher for future developers. This should always be at the forefront of the code reviewer's mindset. It's quite easy for the review process to degenerate into a style argument, quest to find every inefficiency, or attempt to make code as "clever" and brief as possible.

romed 8y ago
I think a key to instilling positive code review culture is to make sure that the comments are based on some objective, such as readability and clarity, and not based on personal preference. In particular, if one person is reviewing code of another, it is not helpful for the reviewer to remark that they would have written it differently, if they had written it at all. Some people for some reason can't keep themselves from doing this. They should remember that they did not, in fact, write the change, so their personal style is irrelevant. If code is clear, obviously correct, well-tested, and correctly formatted, it LGTM.
- gear54rus 8y ago
  except the definition of readability varies and we're back to square one
  - romed 8y ago
    Yes well in the final analysis all problems are recruiting problems, aren’t they?
  - BurningFrog 8y ago
    Not really. Between serious professionals, there will be a few disagreements, but (1) you can work through them with some give and take, and (2) over time you get to know each other, which usually means one persons agrees the other's style is better, or you agree to to disagree on on point.
  - dirkgently 8y ago
    Unless companywide guidelines are agreed upon, and published. E.g http://google.github.io/styleguide/javaguide.html http://google.github.io/styleguide/javaguide.html https://github.com/google/styleguide/blob/gh-pages/pyguide.md https://github.com/google/styleguide/blob/gh-pages/pyguide.m...
    - mikewhy 8y ago
      https://developers.google.com/edu/python/introduction https://developers.google.com/edu/python/introduction: According to the official Python style guide (PEP 8), you should indent with 4 spaces. (Fun fact: Google's internal style guideline dictates indenting by 2 spaces!) https://github.com/google/styleguide/blob/gh-pages/pyguide.md#34-indentation https://github.com/google/styleguide/blob/gh-pages/pyguide.m...: Indent your code blocks with 4 spaces.
- deleted 8y ago
  [deleted]
- falsedan 8y ago
  I think putting opinions in code review comments is fine, as long as you’re clearly indicating it as an opinion. The worst is when a reviewer tries to justify their opinionated comment with some vague appeal to style or readability instead of “I hate this part, ship it”
specialist 8y ago
Reading another human's code (or prose) is the closest thing we have to mind reading. Whenever I can't avoid doing code reviews, most often, I really wish I didn't know that much about the author. -- "...code must act as a teacher..." Agree. Alas, most teachers aren't very good. https://en.wikipedia.org/wiki/Sturgeon's_law https://en.wikipedia.org/wiki/Sturgeon's_law I used to really care about this stuff. I started a design pattern study group 25 (?) years ago. It's still going strong. (Geeks love to eat pizza and argue about the rules. :) ) Professionally, I've all but given up. Any more, I just try to mimic the code style of whatever goo I'm working on. And do whatever kabuki required of me to get my PRs merged. Programming is now my hobby. Whenever I feel the need to code with intent and feeling, I work on a personal project. (For some reason, this reminds me of Al Pacino quote about exercising. Something like "I just lie down until the feeling goes away." Happily, I'm not that bleak.) -- When I am feeling optimistic, I imagine a future where we adopt a hybrid of Pieter Hintjens' Social Architecture and Michael Bryzek's Test in Production. https://legacy.gitbook.com/book/hintjens/social-architecture/details https://legacy.gitbook.com/book/hintjens/social-architecture... https://qconsf.com/sf2017/sf2017/presentation/testing-production-quality-software-faster.html https://qconsf.com/sf2017/sf2017/presentation/testing-produc... TL;DR: Accept all PRs, radically reduce the cost of changes.
- Aeolun 8y ago
  I think I mostly accept anything that comes my way, except for ones that have obvious, non-blocking fixes. People tell me to be more critical, but there’s no point in holding everything up because someone wrote a triple nested ternary operator. It’s icky, but they, or someone else, will fix it the next time they come across that piece of code. That’s why we have tests.

romed 8y ago

I think a key to instilling positive code review culture is to make sure that the comments are based on some objective, such as readability and clarity, and not based on personal preference. In particular, if one person is reviewing code of another, it is not helpful for the reviewer to remark that they would have written it differently, if they had written it at all. Some people for some reason can't keep themselves from doing this. They should remember that they did not, in fact, write the change, so their personal style is irrelevant. If code is clear, obviously correct, well-tested, and correctly formatted, it LGTM.

gear54rus 8y ago
except the definition of readability varies and we're back to square one
- romed 8y ago
  Yes well in the final analysis all problems are recruiting problems, aren’t they?
- BurningFrog 8y ago
  Not really. Between serious professionals, there will be a few disagreements, but (1) you can work through them with some give and take, and (2) over time you get to know each other, which usually means one persons agrees the other's style is better, or you agree to to disagree on on point.
- dirkgently 8y ago
  Unless companywide guidelines are agreed upon, and published. E.g http://google.github.io/styleguide/javaguide.html http://google.github.io/styleguide/javaguide.html https://github.com/google/styleguide/blob/gh-pages/pyguide.md https://github.com/google/styleguide/blob/gh-pages/pyguide.m...
  - mikewhy 8y ago
    https://developers.google.com/edu/python/introduction https://developers.google.com/edu/python/introduction: According to the official Python style guide (PEP 8), you should indent with 4 spaces. (Fun fact: Google's internal style guideline dictates indenting by 2 spaces!) https://github.com/google/styleguide/blob/gh-pages/pyguide.md#34-indentation https://github.com/google/styleguide/blob/gh-pages/pyguide.m...: Indent your code blocks with 4 spaces.
deleted 8y ago
[deleted]
falsedan 8y ago
I think putting opinions in code review comments is fine, as long as you’re clearly indicating it as an opinion. The worst is when a reviewer tries to justify their opinionated comment with some vague appeal to style or readability instead of “I hate this part, ship it”

specialist 8y ago

Reading another human's code (or prose) is the closest thing we have to mind reading. Whenever I can't avoid doing code reviews, most often, I really wish I didn't know that much about the author. -- "...code must act as a teacher..." Agree. Alas, most teachers aren't very good. https://en.wikipedia.org/wiki/Sturgeon's_law https://en.wikipedia.org/wiki/Sturgeon's_law I used to really care about this stuff. I started a design pattern study group 25 (?) years ago. It's still going strong. (Geeks love to eat pizza and argue about the rules. :) ) Professionally, I've all but given up. Any more, I just try to mimic the code style of whatever goo I'm working on. And do whatever kabuki required of me to get my PRs merged. Programming is now my hobby. Whenever I feel the need to code with intent and feeling, I work on a personal project. (For some reason, this reminds me of Al Pacino quote about exercising. Something like "I just lie down until the feeling goes away." Happily, I'm not that bleak.) -- When I am feeling optimistic, I imagine a future where we adopt a hybrid of Pieter Hintjens' Social Architecture and Michael Bryzek's Test in Production. https://legacy.gitbook.com/book/hintjens/social-architecture/details https://legacy.gitbook.com/book/hintjens/social-architecture... https://qconsf.com/sf2017/sf2017/presentation/testing-production-quality-software-faster.html https://qconsf.com/sf2017/sf2017/presentation/testing-produc... TL;DR: Accept all PRs, radically reduce the cost of changes.

Aeolun 8y ago
I think I mostly accept anything that comes my way, except for ones that have obvious, non-blocking fixes. People tell me to be more critical, but there’s no point in holding everything up because someone wrote a triple nested ternary operator. It’s icky, but they, or someone else, will fix it the next time they come across that piece of code. That’s why we have tests.