If people get upset that you "dare questioning them", they're usually not as competent as they'd like to be seen. Treat them as volatile, and if you have questions where you're unsure, ask instead an experienced person you do trust.
But never, ever, rubber-stamp code you don't understand.
Don't punish the apprentice for getting their review "wrong"; just show them the master's annotations and let them learn from them. See where they thought the same things, and where they thought differently. Let them argue with the master if they disagree on their judgement, and let this be an opportunity for learning also (on both the apprentice's and master's parts!)
"Craftwork" is another such word, being "the craft produced as the work of a trade, or the process of applying one's craftsmanship to produce work."
Both, I think, are useful in a programming context. As programmers, we do craftwork, and we need to know our tradecraft to do it well--where tradecraft is separate from plain fact-knowledge (it's more a set of intuitions picked up with experience), but is also separate from raw craftsmanship (which is composed of things like willingness-to-say-no-when-told-to-ship-something-unmaintainable, but which doesn't cover the trade-specific knowledge required to say what "unmaintainable" looks like.)
"Uncle" Bob Martin's writings about code craftsmanship (http://blog.8thlight.com/uncle-bob/archive.html) are exactly that--teaching coders about what craftsmanship, in general, is. He doesn't really talk about tradecraft, just about encouraging coders--who already have implicitly learned tradecraft--that they should be considering themselves craftsmen, and applying their tradecraft.
On the other hand, for all the twee style of The Codeless Code (http://thecodelesscode.com/), it's basically a set of parables that do teach tradecraft.
I don't understand the relevance of "trade" to this discussion. It's just craft, or craft-knowledge, if you prefer.
One approach might be this rule: "If you don't understand, get an explanation from whoever it came from until you do understand."
If there's a more-senior reviewer available, you might have them come along for the discussion to ask additional questions as necessary, but the main focus should be getting the original developer to explain their reasoning to the junior reviewer.
Also, if it's not obvious on a cold reading, it has a good chance of being poorly designed in some way as you've discovered.
Just like there's a rule "you're never the smartest person in a room, there's probably someone smarter than you" there's a rule "you're never the stupidest person in the room, there's probably someone stupider than you". Someone in the future will be confused too.
Just push for a comment.
Unfortunately not every developer is comfortable with it, often it's cultural thing at play (talking about developers from countries like India).
My biggest pet peeve when I send code out for review is someone responding with the superficial bullshit stuff, so it's great that you identified it as a problem. You do tend to get reviews like that from junior folks, but I also see it with really insecure engineers. People start using code reviews as a weapon to take pot shots at each other.
I also think it becomes a problem when you're doing these smaller code reviews as the blog post is recommending; if you're doing bite sized chunks of code, it's really easy to lose context and overlook things which might be performance bottle necks.
Absolutely. In fact not just performance bottlenecks but other things too. Worse still is if a commit is not a discrete fix, then you have to review a set of commits, possibly editing and re-editing the same code, together.
Additionally I imagine I would have less trouble with a single 100 line commit than I would with 100 one-line fixes. Context tracking has overhead.
the single best rule is not to worry about size of the commit but ensuring that the commit is self-contained and tested (generally, not necessarily perfectly) before being committed. Commit things so that they are discrete and reviewable chunks and worry less about size.
If you will, then why not do it before the review and spare everyone the trouble? In a codebase that conforms to certain standards steps away from the standard stick out and distract from the content.
If you won't, then maybe it's good that people point it out so that you can fix it manually?
Of course there is a difference between a comment saying: "Nit: Trailing whitespace" and "According to Section V, Subsection VII of the Coding Manual you should never add trailing whitespace. Please see that you don't." or some stuff like that. The latter is a passive-aggressive potshot, the former IMO is just a quick reminder.
Send something out with a fairly complex algo and you get back people complaining about a brace being 2 spaces instead of 4 or something else that will be automatically cleaned up by your editor before commit.
If you decide to not stick to the style, you're essentially saying "I don't care"/"I'm sloppy". It's a red flag that there might be other issues. It means you don't care about sweating the small details.
We have a very strong culture of constructive code review though - I guess you might have some work to do to get those more experience coders to get into the mindset that would accept this sort of feedback in a positive way
I'm a relatively senior engineer in my department, and I frequently have more junior engineers doing my code reviews. A lot of the time when they point out "What's this for?" or "Why didn't you do it this way?", it really is an error or an oversight, and I should've done it that way. Senior != omniscient. And in cases where I do have a good reason for what I did, it's a teaching opportunity, where I can make the code reviewer aware of a complexity that hasn't come up.
1. Review collaboratively. If you have questions, ask other reviewers and the developers. By asking questions you aren't questioning their wisdom, just pointing out that you don't understand what they wrote, so be respectful.
2. Introduce new programmers to the process gradually and require more collaboration than you would for more senior programmers. This helps ensure that new people are brought up to speed with appropriate mentoring from their senior staff.
Review is a form of collaboration. It is a way to shake out logic before stuff gets into formal testing. Discussion is good.
For awhile, this resulted in me effectively checking to be sure it looked 'grammatically correct'- as in, followed style guides, was linted right, etc. The bullshit stuff.
Then a few got through where I had reviewed them, wrote down "What's this for?" as a thing to review a month or so later, and they turned out to be huge problems- sometimes performance, sometimes functionality that would break under the right circumstances.
Have others gone through this, and if so, how would you recommend 'teaching' a newer coder how to do quality code reviews of the elder coders around him?