Hacker News Reader

Comment by quaunaut | Hacker News Reader

As a less experienced coder reviewing coders with half-decades or more experience on me, I always had a difficult time knowing whether I was looking at something that I just didn't understand the purpose of, or if I should question their wisdom.

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?

View on HN

Speaking as an "elder" (Yikes!) - do ask questions. Worst case, you'll learn something. And most people who have been coding for a while are happy to explain things, because we know that it a) might help expose problems we thought we had covered, and b) makes sure that there's one more person who knows how to do things, and why stuff is the way it is.

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.

I certainly came across people(so far, universally my bosses- lead developers who I didn't directly report to so far haven't been like this at all) who certainly had the "don't dare question me" attitude. It made it a lot more difficult to go to them when I was genuinely stuck too, because in general you always feel like you're getting in their way.

As in any tradecraft: have the apprentice review it. Then, have the master review it. Check the apprentice's work against the master's.

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!)

I think you mean "trade or craft"; "tradecraft" means "spying" (in my experience).

"Tradecraft" is a general word meaning "the craft-knowledge involved in a trade"; it just isn't very well-used out of a few specific contexts.

"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.

> "Tradecraft" is a general word meaning "the craft-knowledge involved in a trade"; it just isn't very well-used out of a few specific contexts.

I don't understand the relevance of "trade" to this discussion. It's just craft, or craft-knowledge, if you prefer.

I disagree that "tradecraft" is still so general. I agree "craftsmanship" has no such connotations.

I spent 12 years in the Air Force, the last 4 in an intel unit writing software to analyze radar signal intercepts. I've never heard the word tradecraft used to mean anything intel-related, although Google tells me that it is used that way. It may be more common in some intel communities than others, but I'd venture to guess that if you did a survey the large majority of folks in the general population will have no idea about the intel-related usage of the word.

I wasn't particularly claiming it was used by a large portion of those in intelligence; my claim was that it was an overwhelmingly large portion of the uses of the term, and those familiar with it at all (and not just guessing the meaning based on the fact that it's made up of two familiar words) are most likely to have heard about it in Burn Notice (which apparently had ~5 million viewers) or read it in a Tom Clancy novel (of which there may be 100 million copies in print), if they didn't read it in a recent Al Jazeera or Washington Post headline.

> 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?

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.

Very much so. Either it's a bug, it's not written clearly, or you have something to learn. Any which way it's a problem, and asking about it is the way to fix it.

As a junior, you're expected to ask stupid questions. Despite what you may fear, otherwise you can't learn fast enough and it's too expensive to keep you around.

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.

The answer is REALLY simple here. Say "I don't understand what this is doing" and then have add a comment.

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.

Instead of writing something down "What's this for?", go ask the person "What's this for?". I have recently become a primary reviewer for our operating system work (device drivers for SAN storage, VLAN networking, init scripts, custom rpms, etc) which involves a whole lot of bash magic and linux system stuff I do not understand. What I do is go ask the guy to explain to me the basic concepts and/or specifics of the change if I don't understand it. That way it's at least possible that an error will be caught, and when I have to go make changes in that area I will have some idea of what I am up against. Code review is a great time to learn more about what you don't understand, and for more eyes to see dark parts of the code. It's also a good chance for the guy to wrote it to go "hey, that's weird..." when explaining it to me, and realize he missed something important.

This. TL;DR: it's really important to be comfortable with saying "I don't know" and asking questions.

Unfortunately not every developer is comfortable with it, often it's cultural thing at play (talking about developers from countries like India).

Also keep in mind that there is value in being a rubber duck here. There's also a good chance that in explaining it to you, he will find an error during a kind of review that he wouldn't on his own code otherwise.

You can ask the senior engineers, but nothing beats rolling up your sleeves, diving in and peeling the onion back. If you have a good manager, you need to tell them you need more time to absorb and understand the code base you're working on. They should be more than willing to accommodate, particularly for a more a coder with less experience.

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.

> 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.

I dunno, either you will reformat using some kind of automated tool before you check in or you won't.

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.

My biggest pet peeve when I send code out for review is someone responding with the superficial bullshit stuff

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.

You should have an automatic code formatter / checker if possible. Plus code linter. Automate everything if possible and make build fail in case of discrepancies. Then there'll be no discussions like that.

Counter point: We, the team, agreed on a coding style for a reason.

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.

I used to worry about this too, but finally concluded that me not understanding it was in and of itself grounds for rejecting that review. 90% of the time well structured code (and tests, and good comments, etc etc) should be enough on their own to make it easy for me to understand. In the rare cases when the nature of the change means that's not enough then people ideally provide any additional explanation in a note accompanying the review (or in a crucible comment). (In practice I reckon most of the time these explanatory notes probably belong in code comments anyway.)

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

You should always be able to ask "What's this for?", and a senior engineer should be able explain what it's for.

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.

Two things I have found helpful:

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.

It never hurts to ask "Hey, what's the code here supposed to do, it looks complex/off?" :)

Reply on news.ycombinator.com