Today I was talking to one of my co-workers about some janky code we have in our system. And no, Amazon is not unique, there’s janky-ass code all over the place… it gets the job done but does have costs associated with it.
“But your team did a code review on it…”
There’s the problem with code reviews (CRs) (and approvals for deployments, in Amazon they’re called “Change Management” or “CMs”)… it mostly seems that they are used as a way of deflecting blame when something goes awry. The thing with CRs is that they mostly can catch simple mistakes like spelling and whatnot. In the times I’ve been doing CRs, and having them done on my code, there are, of course, plenty of times that things got better from them, so I’m not saying they shouldn’t be done. But the problem is that CRs are generally too far into the weeds to catch major structural issues in the code.
One of the key problems is that you’re reviewing only the code that’s changed, plus a few lines on either side. You can look at the code and things seem like they are doing the job right. You can even see other code that might be similar that had already been reviewed and think to yourself “well, just following the pattern…”
Then you get to work on the system as a whole. Just because every line of code has been signed off on by one or more people does not mean that the code is any good.
You should never look at the code and blame the people that did the CRs for the lack of quality.
The buck ought to stop with the author.