6 ms·
My observation is that code reviews are one of the worse solutions to the problem. They are almost never done right. People tend to treat them as nuisance and n
by twawaaay 4y ago
My observation is that code reviews are one of the worse solutions to the problem. They are almost never done right. People tend to treat them as nuisance and not focus or focus on the wrong things. Or people try to be nice to other people and "not make problems". They are constant source of problems because they tend to crash into deadlines and usually lose. It is also frequently the first time somebody finds out somebody else spend a week writing a feature producing completely unusable mess which could have been avoided if there was human contact during development. Also reviewing large body of work after it has been "completed" is simply the worst time to have open and critical discussion about quality.
My tested and recommended process is working in pairs, rotating pairs regularly and sharing/rotating all tasks within pairs, equally. You have two people actively working together on each piece of code. They are supposed to both understand what is happening, care for quality and be vigilant to catch defects. This is also the right time to have actual input regarding quality or design improvements. And the code once it is done can immediately be committed and shipped so there is no awkward delay between code finished and deployment.
I also learned over the years pair programming is also a fantastic way to achieve some other goals like people socialising, getting to know each other professionally, learning from each other, newcomers getting bootstrapped, etc.
- jvans 4y agoPair programming appears to management as a huge waste — two people doing 1 job!! — but having done both I believe the exact opposite. It increases bus counts, speeds up shipping code(no waiting for review), and improves quality. It is very difficult to ramp someone up on your hard problem you've been working on for days in a short time. If someone is there in the weeds with you, it's so much easier to talk it through. Pull requests degrade to nit picks that are irrelevant to code quality and code function to make the reviewer feel like they are contributing
- peterangular 4y ago> Pair programming appears to management as a huge waste — two people doing 1 job! Sorry to play devil's advocate, but pair programming is literally two people doing a job that has traditionally/historically been done by one person. There is no surprise this is going to cause raised eyebrows with management. Two devs sharing a keyboard is 100% going to be seen as two people fulfilling a single person's job to nearly anyone external to the role for obvious reasons.
- l0b0 4y agoAnother way to look at is is two people doing two jobs at the same time - programming and code review. Or even three jobs, if you include the (IME incredibly useful) design review which seems to happen almost inevitably as part of the implementation.
- tharkun__ 4y agoAnd it's incredibly one sided. If you and your peers like it, use it. I happen to hate it. Tried it at multiple companies and it still makes no sense to me. You are forcing two people to do a specific thing at the same time and pace. One of them has a meeting? Sorry can't do any work. Oh the other guy has a meeting after yours is done? Can't do any work. Oh one of them has a doctor's appointment? No coding. I prefer "the best of both worlds". Workshop/whiteboard the solution. Go separate ways if possible working on different parts of the problem. A great split is someone doing the frontend and the other doing the backend if that's the kind of thing you are working on. Or find some other way to parallelize. Reconvene after some hours/when you are done with something agreed upon or if "stuck". Not everyone likes 100% of the time social contact. Sometimes I just really need to be left alone in a corner, head down, no distractions.
- twawaaay 4y agoPair programming is definitely not for everybody. Some people hate it for multiple possible reasons -- because they can't focus with other person talking, because even if they stop talking the awkward silence is distracting, because they are distracted by their feelings about putting up half baked code and opening their development process to another person, etc. What I do is that I include pair programming as part of interview process to let people experience the process a bit before they decide to join the team. Can't do much about people who are already in the team -- I try to be flexible about it but only to some extent. In the end, being part of large software development project is a social endeavour, whether you like it or not. I get that some people prefer to work alone but I have to insist that social part cannot be somehow ignored.
- rgblambda 4y ago>to make the reviewer feel like they are contributing I swear maybe 50% of review comments are either to make the reviewer feel like they aren't a rubber stamp or just to signal that they actually read the PR.
- JackMorgan 4y agoI completely agree with you. The sad thing is I've almost always seen management understanding the value of pair programming, but the devs hate it. It's so rare to find developers who are willing to pair for even half the week.
- dhab 4y agoI agree. I am a dev, and I have done pair programming few times now. I am not sure I got the value that was hyped. I'd use that selectively on some nutty areas, and address things more broadly by introducing the "waste" of having to do code-review and KT later.
- dajonker 4y agoI agree about code reviews not being done "right", for my own subjective opinion of what a "right" code review is. I've been in situations where code review was required but effectively useless. Some would only focus on code style and completely miss obvious logic errors or lack of new tests. Some would blindly approve anything. Sometimes a manager would override a critical code review because a new feature had to be released urgently (and of course it turned out things were indeed bad later). Sometimes a one character bugfix would be blocked for an hour by the lack of code review.