3 ms·

I've worked with pre-commit code review, post-commit code review, and no code review, and I strongly prefer pre-commit. Surprisingly often, there are issues tha

by jefftk 6y ago

I've worked with pre-commit code review, post-commit code review, and no code review, and I strongly prefer pre-commit. Surprisingly often, there are issues that come up in code review that require the change to be completely rethought, in which case you really don't want the change to have already been submitted. People, especially junior reviewers, are also much less willing to ask for substantial changes post-commit, because it's so much additional work. It's a good fit for situations where code reviews are just a quick check that there's nothing seriously wrong with the commit, but if you're using your code reviews as the excellent teaching / mentoring tool that they can be, code reviews aren't a rubber stamp.

mcny 6y ago
> code reviews aren't a rubber stamp I still remember my first "real" job. The team was on TFS and my code was basically if (condition) { if (same condition) { // do something } } and a senior developer laughed and said mcny is testing whether we actually look at the code and not just rubber stamp his changes. It was actually an honest mistake. At the same place though, we had F5 load balancers and actual integration with AS/400 systems but development/test environments didn't have a load balancer in front of them :(
- yjftsjthsd-h 6y ago
  > At the same place though, we had F5 load balancers and actual integration with AS/400 systems but development/test environments didn't have a load balancer in front of them :( That sucks; I really wish people would make a better effort at staging-production parity. Like, it's one thing to run dev on a smaller cluster or older hardware (although still potentially risky!), but changing out things like how connections are passed to the environment seems like asking for incompatible changes to get pushed through and break prod.
  - WrtCdEvrydy 6y ago
    But how will our developers unlock a sense of pride and accomplishment if they can't rely on the underlying infrastructure to be consistent between PROD and QA...
  - dijit 6y ago
    >That sucks; I really wish people would make a better effort at staging-production parity. Like, it's one thing to run dev on a smaller cluster or older hardware ( I can't speak for everybody, but, honestly I've always fought for parity of dev and prod; usually it comes down to cost though. If infrastructure is 10% of the operating budget of something and you need a dev environment for every team (or, ever dev!) then that cost is going to explode.
- Cthulhu_ 6y ago
  > testing whether we actually look at the code and not just rubber stamp his changes I've actually put in some er, "brown M&Ms" in my code before sending it off to review, because I was sure the people reviewing my code were like "oh, he wrote it so it's probably good". I think what would've helped in that case would be anonymized code reviews & comments, where the names of the author / commenters only pops up after it has been approved & merged. Actually I think that should be a feature everywhere, e.g. in Github issues. That way it becomes harder to judge e.g. comments / issues / commits based on the person who made it. Pseudonyms are nice, but you still build up a 'reputation' over time. But it'll probably have to be an opt-in option per user.
z3t4 6y ago
Saving commits are good for code archeology. Like initial implementation did not work because
- u801e 6y ago
  Those commits do not necessarily need to be in the history. The commit with the working implementation could explain that X alternate solutions were considered but those solutions didn't work with the details of why they didn't work. Of course, that's assuming that those other solutions were tried, tested, and found not to work prior to them getting merged and deployed in production.
  - WorldMaker 6y ago
    On the flipside, git is based on a full DAG. There's a lot of power within that data structure to capture a deep, nested understanding of the workflow/process/history/archaeology. Often the issue is not that the DAG can't usefully hold all the alternates and piecemeal sub-solutions, in useful ways that we can come back to and revisit as necessary, but that our visualization tools on top of the DAG aren't great about letting us meaningfully interact with that (showing it when we need it, hiding it when it is noise to the signal we are looking for). (And we have the power to build better defaults for our visualization tools and/or better visualization tools.)
  - z3t4 6y ago
    Code is more precise then prose. Someone could say "we already tried X" then you can test it yourself. Repeating an experiment could be as easy as a git checkout.
- jefftk 6y ago
  Initial versions, and the review comments that led to their abandonment, can be saved in your code review system.
  - WorldMaker 6y ago
    What if you need to change code review systems? Do you just abandon all that information? What if your code review system becomes a central point of failure for information that you need about your process/workflow? To use a DVCS in a distributed fashion, doesn't it make more sense to store that data in the DVCS itself?

by jefftk 6y ago

mcny 6y ago

> code reviews aren't a rubber stamp I still remember my first "real" job. The team was on TFS and my code was basically if (condition) { if (same condition) { // do something } } and a senior developer laughed and said mcny is testing whether we actually look at the code and not just rubber stamp his changes. It was actually an honest mistake. At the same place though, we had F5 load balancers and actual integration with AS/400 systems but development/test environments didn't have a load balancer in front of them :(

yjftsjthsd-h 6y ago
> At the same place though, we had F5 load balancers and actual integration with AS/400 systems but development/test environments didn't have a load balancer in front of them :( That sucks; I really wish people would make a better effort at staging-production parity. Like, it's one thing to run dev on a smaller cluster or older hardware (although still potentially risky!), but changing out things like how connections are passed to the environment seems like asking for incompatible changes to get pushed through and break prod.
- WrtCdEvrydy 6y ago
  But how will our developers unlock a sense of pride and accomplishment if they can't rely on the underlying infrastructure to be consistent between PROD and QA...
- dijit 6y ago
  >That sucks; I really wish people would make a better effort at staging-production parity. Like, it's one thing to run dev on a smaller cluster or older hardware ( I can't speak for everybody, but, honestly I've always fought for parity of dev and prod; usually it comes down to cost though. If infrastructure is 10% of the operating budget of something and you need a dev environment for every team (or, ever dev!) then that cost is going to explode.
Cthulhu_ 6y ago
> testing whether we actually look at the code and not just rubber stamp his changes I've actually put in some er, "brown M&Ms" in my code before sending it off to review, because I was sure the people reviewing my code were like "oh, he wrote it so it's probably good". I think what would've helped in that case would be anonymized code reviews & comments, where the names of the author / commenters only pops up after it has been approved & merged. Actually I think that should be a feature everywhere, e.g. in Github issues. That way it becomes harder to judge e.g. comments / issues / commits based on the person who made it. Pseudonyms are nice, but you still build up a 'reputation' over time. But it'll probably have to be an opt-in option per user.

z3t4 6y ago

Saving commits are good for code archeology. Like initial implementation did not work because

u801e 6y ago
Those commits do not necessarily need to be in the history. The commit with the working implementation could explain that X alternate solutions were considered but those solutions didn't work with the details of why they didn't work. Of course, that's assuming that those other solutions were tried, tested, and found not to work prior to them getting merged and deployed in production.
- WorldMaker 6y ago
  On the flipside, git is based on a full DAG. There's a lot of power within that data structure to capture a deep, nested understanding of the workflow/process/history/archaeology. Often the issue is not that the DAG can't usefully hold all the alternates and piecemeal sub-solutions, in useful ways that we can come back to and revisit as necessary, but that our visualization tools on top of the DAG aren't great about letting us meaningfully interact with that (showing it when we need it, hiding it when it is noise to the signal we are looking for). (And we have the power to build better defaults for our visualization tools and/or better visualization tools.)
- z3t4 6y ago
  Code is more precise then prose. Someone could say "we already tried X" then you can test it yourself. Repeating an experiment could be as easy as a git checkout.
jefftk 6y ago
Initial versions, and the review comments that led to their abandonment, can be saved in your code review system.
- WorldMaker 6y ago
  What if you need to change code review systems? Do you just abandon all that information? What if your code review system becomes a central point of failure for information that you need about your process/workflow? To use a DVCS in a distributed fashion, doesn't it make more sense to store that data in the DVCS itself?