|
Sometimes when I read these things, I wonder if I've just gotten really lucky and selected positions at places with well-adjusted, mature, talented software developers? This is in no way representative of any of my experiences both giving and receiving code reviews. I've experienced them in a variety of "ways" -- senior's reviewing mid/juniors "by policy", dedicated teams reviewing certain types of code for certain purposes[0], "randomly distributed" reviews among people of similar skill levels, two-person I review you, you review me always and on and on. I have been doing this work for far longer than I care to admit. My rejections/non-approvals (both received and provided) land in one of the following categories: (a) Did you notice that? (Oops, D'Oh!), (b) Did you know about that? (Educational), (c) Did you think about that? (Edge Cases), and (d) Why? I work with these men and women day-to-day, so we don't often need to butter up negative responses, we have established that "I trust you can do your job" and my comments do not imply otherwise. The best way I've heard it said is "We do code reviews because errors in software development are common, regardless of proficiency. I don't write perfect code and having a second set of eyes on my work will only make it better/me look better. Reading a growing code-base by reviewing each other's work benefits everyone." Of the ways those land, the (b) category is the one that requires a little care (if your goal isn't just to bully/shit on someone's gap in knowledge). I find the way to take the sting out of it is to start off with "Hey, I came across an approach to solving this using (whatever); have you done anything with that? It would be an improvement ... (comment goes on)". I didn't say I came across that approach 15 years ago, that it's what I have decided is "Kindergarten in C# School". Guess what? Someone's read my code and thought the exact same thing. I know this because I've thought them about code I've written as early as 6 months prior. Of course, it's a matter of perspective. I thought that over the one thing I saw that made my toes curl up, never mind that the rest of it was perfectly fine, just like the code I'm currently reviewing. My favorite, though, is the occasional class/implementation that lands in "Why?". Usually those look like the previous -- a WTF-worthy implementation that the only comment I can send with is "Why was this particular implementation used?" -- and often it requires refactoring ... but every once in a while there's that gem. Some edge case that was cleverly discovered, covered and the developer was left with "This is, literally, the only way that this can be done (given the constraints)." It's the code that's both frighteningly ugly/really cool once you realize why it had to be that way. [0] I was one of two (but really, one of one) doing the entirety of secure code review for a large multi-national telecom for one brief, terrifying, point. |