5 ms·

I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be sh

I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code.

trgn 7y ago
Yes! Engineers should collaborate on the overall approach to a code change/addition with the reviewers before any PR is submitted. Those early discussions are extremely important since teams will converge to an appropriate solution quicker because the process is much more informal. These frequent early discussions also build team spirit, and if you're so lucky, individuals will start to click and the team will start writing code and designing software in similar ways. These early discussion are generally more jovial. The actual review can focus on details, and, if feedback was gathered and put into practice early, nobody will mind the occasional nitpick about punctuation and naming. These will feel earned, even welcome, the PR being a final touch of polish.
- pjm331 7y ago
  Out of curiosity how do those conversations happen before the PR? Asking because we might have a different process for PRs or a different definition of what it means to submit a PR - on many teams I’ve been on it’s been encouraged to publish a WIP PR in order to facilitate these conversations. GitHub recently added a feature to formalize that but before this we used labels such as “WIP” and “Ready for review”
  - trgn 7y ago
    In person, or for a remote job, with screensharing and online chats. In my current job, it generally starts with the implementer reaching out and saying, I want to achieve xyz and think about doing abc. That's how the conversation gets started. Granted, sometimes you need code examples already ready-made to gather meaningful feedback, but that can be personal branch, one-off patch, gist, WIP PR ... feedback can happen over slack, simple chat, comments on the WIP PR ... As long as the final ready-for-review PR doesn't catch the reviewers off guard in terms of overal design/approach.
  - underwater 7y ago
    WIP PRs are fine. Essentially the PR is used as a mechanism to clearly communicate an idea, rather than containing code that is intended to be merged into the codebase. There shouldn't really be a PR review where the author is seeking input on naming/testing completeness and major design choices. Those two types of reviews are mutually exclusive. Alternatives to WIPs include whiteboard discussions, pair programming, and written proposals.
  - yoz-y 7y ago
    For smaller changes it can happen over a coffee in the hallway. For large changes there are design reviews. For stuff in between you can schedule a meeting with a few concerned people.
ergothus 7y ago
Is that a productive idea to push? Not to say you're wrong, but some hard human problems to solve are: - getting people to not be defensive during code reviews - getting people willing to be critical (constructively) of their peers work Emphasizing that code reviews with productive design comments indicates a failing seems more likely to stop the comments, not to improve the design. Most people wont want to do the work multiple times and will quickly learn to do as you desire in the face of repeated code reviews that have comments pointing out design problems.
- Mvhsz 7y ago
  There are times that I get into the flow while coding and end up solving a few future problems in addition to right now problems. I know that the code is over-engineered, but if it passes all of the tests and there's nothing obviously wrong with it I would probably check it in anyway. Over engineering never comes up in our code reviews but I think it would be a productive conversation if it did - even if the decision was still to accept the code as-is. The programmers I work with can be defensive of their architecture, but they also love to talk about architecture generally. I think there's a "yes and" [0] way to bring this up that engages the whole team. [0] https://en.m.wikipedia.org/wiki/Yes,_and.. https://en.m.wikipedia.org/wiki/Yes,_and...
  - onemoresoop 7y ago
    And then you decide to get another job and next guy to maintain the code is utterly confused and misdirected because let’s be honest, there’s no documentation or there is but is either not up to date or sparse, theres no thorough unit testing (lucky if there is any). Not saying that’s your case but it happened to me personally to be on the other end a few times and let me tell you, it’s quite an effort to ramp up on such projects.
scottlamb 7y ago
> I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code. Sometimes a prototype can help shake these things out, though. It's often okay to go an extra stage beyond what's been reviewed/approved (e.g., send out a prototype implementation before design review is complete) if you're willing to throw it away. If you'd be upset by someone saying "try this better design", you need to wait for a design review before writing any code. Another thing to be careful about with a prototype is keeping it as far away from the normal production serving path as possible to avoid compromising the essentials of reliability, privacy, and security. (Ideas along those lines: separate production role, separate server, flag-enable it only in a test environment, dark-launch it, experiment-gate it, etc.)

trgn 7y ago

Yes! Engineers should collaborate on the overall approach to a code change/addition with the reviewers before any PR is submitted. Those early discussions are extremely important since teams will converge to an appropriate solution quicker because the process is much more informal. These frequent early discussions also build team spirit, and if you're so lucky, individuals will start to click and the team will start writing code and designing software in similar ways. These early discussion are generally more jovial. The actual review can focus on details, and, if feedback was gathered and put into practice early, nobody will mind the occasional nitpick about punctuation and naming. These will feel earned, even welcome, the PR being a final touch of polish.

pjm331 7y ago
Out of curiosity how do those conversations happen before the PR? Asking because we might have a different process for PRs or a different definition of what it means to submit a PR - on many teams I’ve been on it’s been encouraged to publish a WIP PR in order to facilitate these conversations. GitHub recently added a feature to formalize that but before this we used labels such as “WIP” and “Ready for review”
- trgn 7y ago
  In person, or for a remote job, with screensharing and online chats. In my current job, it generally starts with the implementer reaching out and saying, I want to achieve xyz and think about doing abc. That's how the conversation gets started. Granted, sometimes you need code examples already ready-made to gather meaningful feedback, but that can be personal branch, one-off patch, gist, WIP PR ... feedback can happen over slack, simple chat, comments on the WIP PR ... As long as the final ready-for-review PR doesn't catch the reviewers off guard in terms of overal design/approach.
- underwater 7y ago
  WIP PRs are fine. Essentially the PR is used as a mechanism to clearly communicate an idea, rather than containing code that is intended to be merged into the codebase. There shouldn't really be a PR review where the author is seeking input on naming/testing completeness and major design choices. Those two types of reviews are mutually exclusive. Alternatives to WIPs include whiteboard discussions, pair programming, and written proposals.
- yoz-y 7y ago
  For smaller changes it can happen over a coffee in the hallway. For large changes there are design reviews. For stuff in between you can schedule a meeting with a few concerned people.

ergothus 7y ago

Is that a productive idea to push? Not to say you're wrong, but some hard human problems to solve are: - getting people to not be defensive during code reviews - getting people willing to be critical (constructively) of their peers work Emphasizing that code reviews with productive design comments indicates a failing seems more likely to stop the comments, not to improve the design. Most people wont want to do the work multiple times and will quickly learn to do as you desire in the face of repeated code reviews that have comments pointing out design problems.

Mvhsz 7y ago
There are times that I get into the flow while coding and end up solving a few future problems in addition to right now problems. I know that the code is over-engineered, but if it passes all of the tests and there's nothing obviously wrong with it I would probably check it in anyway. Over engineering never comes up in our code reviews but I think it would be a productive conversation if it did - even if the decision was still to accept the code as-is. The programmers I work with can be defensive of their architecture, but they also love to talk about architecture generally. I think there's a "yes and" [0] way to bring this up that engages the whole team. [0] https://en.m.wikipedia.org/wiki/Yes,_and.. https://en.m.wikipedia.org/wiki/Yes,_and...
- onemoresoop 7y ago
  And then you decide to get another job and next guy to maintain the code is utterly confused and misdirected because let’s be honest, there’s no documentation or there is but is either not up to date or sparse, theres no thorough unit testing (lucky if there is any). Not saying that’s your case but it happened to me personally to be on the other end a few times and let me tell you, it’s quite an effort to ramp up on such projects.

scottlamb 7y ago

> I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code. Sometimes a prototype can help shake these things out, though. It's often okay to go an extra stage beyond what's been reviewed/approved (e.g., send out a prototype implementation before design review is complete) if you're willing to throw it away. If you'd be upset by someone saying "try this better design", you need to wait for a design review before writing any code. Another thing to be careful about with a prototype is keeping it as far away from the normal production serving path as possible to avoid compromising the essentials of reliability, privacy, and security. (Ideas along those lines: separate production role, separate server, flag-enable it only in a test environment, dark-launch it, experiment-gate it, etc.)