3 ms·
Our formal policy is that at least one other team member must review a change before it is checked in. My impressions: 1. Many problems get fixed before the
by keypusher 14y ago
Our formal policy is that at least one other team member must review a change before it is checked in. My impressions:
1. Many problems get fixed before the review even goes out. When you know others are going to be looking at the code, you tend to take a much closer look before sending it out. Also, you tend to clean up those quick hacks you put in to "just make it work".
2. It keeps the team informed. There's always at least one other person out there that knows what you have been working on. This gives you someone to bounce ideas off, but it also keeps a team together, and forces people out of getting silo'd into one particular area.
3. It is often more valuable to send out a review before it's done. If you are working on a new framework / library / class, it's better to design and prototype it, then send out a review to some people for feedback instead of waiting until you are done. They might point you in a different direction, or offer criticism, but it's much better to hear that before you have spent weeks on implementation than after.
4. Make the review process universal. If it isn't required, people won't do it. And it should be required for everyone, not just the new guys. Senior people can still learn things too, and if they are really writing great code then it should be shared with the team as an example of how to do things right.
5. Make the review process painless. Make it easy for people to generate diffs that can be shared and easy to comment, there are good tools out there for this. If the barrier to checking in changes is too high, a lot of easy fixes will get ignored because people just can't be bothered.