4 ms·

Honest question: A couple of years ago I worked for a startup. They used Agile, which mandates code reviews. "Brilliant", I thought, "that can only be good." I'

Honest question: A couple of years ago I worked for a startup. They used Agile, which mandates code reviews. "Brilliant", I thought, "that can only be good." I'd never really had code reviews before even though I've got decades in industry. I like the idea. However there was no written (or often, even oral) statement of what it was supposed to do so all I could do was spot syntactic cruft and offer cleaner code, and... that was about it. To me, without clear statement of code's intent it must be impossible to review that code meaningfully, but I've never come across this most basic requirement in Agile that the code must have a spec of some sort to review against. What am I missing, or was their Agile practice simply wrong, or what? Genuine question.

Benjammer 8y ago
Was this someone on your direct team? Were you not part of the spec-review/sprint planning/etc meeting(s) where the project goals for the code you're now reviewing was discussed? If someone just dumped a PR in your lap with no context and said "do a code review," then this sounds like just a really terrible process/implementation. The idea, imho, is that you are supposed to be intimately familiar with all the projects on your team, while only coding for your own. If you each follow each other's tech doc/spec, and actually put real, professional effort into good technical writing and understanding of your team members' communication styles, you can all have a shared understanding of the problem and the solution architecture/idea, and then after the code is written, code review becomes a process of aligning on what the "right" solution is for the team, given hyper specific context of the one PR/feature/project/bug fix/whatever. Did it turn out like everyone expected? Why or why not? Does this align with the maintainability goals of our team? What will ongoing support for this look like? Does it move the needle for (any) clients? Does it unblock other projects/tickets? etc... I don't really agree with the type of code review where you nit pick about adherence to arbitrary "style guides," read the code line by line to "try and find bugs," or asking for changes because you think something could be "cleaner" or whatever. The only code reviews that have real impact in my mind are the ones focused on how the code maps to real business goals.
nradov 8y ago
"Agile" doesn't mandate code reviews. However many agile teams find that code reviews are helpful to improve quality and increase velocity. The user story acceptance criteria (confirmation) provides the clear statement of the code's intent. Every code review should be tagged with a specific user story number. (If the user story isn't clear then that's a separate problem.)
tempguy9999 8y ago
@Benjammer: what you say sounds so unlike what happened that I can only take it that our process was a mess. You final line about real business goals summed up a lot of what should have been, but wasn't, considered there. @nradov: Nope, no 'user stories', whatever they are. There were a lot of other bad decisions at the company. Some really stupid stuff. I don't suppose they'll last much longer. Thanks to you both. I'll spend this eve reading up on Agile.

Benjammer 8y ago

Was this someone on your direct team? Were you not part of the spec-review/sprint planning/etc meeting(s) where the project goals for the code you're now reviewing was discussed? If someone just dumped a PR in your lap with no context and said "do a code review," then this sounds like just a really terrible process/implementation. The idea, imho, is that you are supposed to be intimately familiar with all the projects on your team, while only coding for your own. If you each follow each other's tech doc/spec, and actually put real, professional effort into good technical writing and understanding of your team members' communication styles, you can all have a shared understanding of the problem and the solution architecture/idea, and then after the code is written, code review becomes a process of aligning on what the "right" solution is for the team, given hyper specific context of the one PR/feature/project/bug fix/whatever. Did it turn out like everyone expected? Why or why not? Does this align with the maintainability goals of our team? What will ongoing support for this look like? Does it move the needle for (any) clients? Does it unblock other projects/tickets? etc... I don't really agree with the type of code review where you nit pick about adherence to arbitrary "style guides," read the code line by line to "try and find bugs," or asking for changes because you think something could be "cleaner" or whatever. The only code reviews that have real impact in my mind are the ones focused on how the code maps to real business goals.

nradov 8y ago

"Agile" doesn't mandate code reviews. However many agile teams find that code reviews are helpful to improve quality and increase velocity. The user story acceptance criteria (confirmation) provides the clear statement of the code's intent. Every code review should be tagged with a specific user story number. (If the user story isn't clear then that's a separate problem.)

tempguy9999 8y ago

@Benjammer: what you say sounds so unlike what happened that I can only take it that our process was a mess. You final line about real business goals summed up a lot of what should have been, but wasn't, considered there. @nradov: Nope, no 'user stories', whatever they are. There were a lot of other bad decisions at the company. Some really stupid stuff. I don't suppose they'll last much longer. Thanks to you both. I'll spend this eve reading up on Agile.