How to be a faster code reviewer | Hacker News Reader

How to be a faster code reviewer

blog.codacy.com

62 pointsjaimefjorge43 comments

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?

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.

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.

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

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

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

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.

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.

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.

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

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.

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.

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.

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.

The first point isn't something you can do: that's something you others have to do for you. Unless you count convincing people to change their workflow, which I've found to be near equivalent to moving mountains.

In my experience, it seems to boil down to not being effective with their VCS (or in some cases, the VCS not being an effective tool). But it's hard to convince people to go through the trouble of learning something: the short-term cost is just too much, and they seem to be blind to the long term gains. It's like convincing someone to use vim (who isn't an emacs or equivalently-powerful-editor user).

A related question. I'm wondering what other people do to ensure quality of outsourced code. I'm intrigued at Codacy. A lint in the cloud could solve some of my problems, but probably not all.

I'm in the position of occasionally outsourcing non-critical development but not having time myself (I used to be technical; now focused on marketing) or able to distract the dev team to look at code in depth.

Do you outsource development? How often do you code review that code? Never, once in a while, on delivery, frequently? Do you do anything else to ensure outsourced code quality?

My company outsources to Vietnam. At the beginning, we had time to review all the code, and we always had feedback on it. Only after several iterations would it be ok.

We would review the code manually, and later also used tools such as Jenkins and Sonar.

However, as the number of FTEs over there increased and we switched to paying per month instead of per project, the amount of the code we would received became overwhelming (about 5-6 FTEs over there per 1 FTE here), and the focus moved to getting functionality right in time.

Later we would discover horrible things in that code when we'd have to fix some issue. Which was difficult in itself because we did not know the code at all.

Very low code quality and constant issues and delays lead us to start hiring people here and slowly phase out Vietnamese programmers. We (as the local dev team) already feel much more in control of our product.

Hi Sunir. Great questions. Hope to get more people answering this.

Please also take a look at a parallel discussion in proggit: http://www.reddit.com/r/programming/comments/1zqbx1/10_ways_...

From my experience in Codacy, some of our users use it to grade and quickly send out comments from the tool to the outsourced team. They like being able to review the code as it's being done (since it's per commit) with the help of our linting code patterns.

We also give out grades which is an overall vision of the project without having to go into much technical detail. "Why is this a C?" is a great question to ask :) I'd love to hear more about your experience and your pains and check if we can be of assistance. Here's my email: jaime at codacy dot com.

They missed the most important one, practice. It might sound like I am being snarky or stating the obvious, but like any new task at first you will be slow at code review and it will take up "too much time". Its a skill, in some situations, its not really too much code to review once you have gotten good at reading code. Everyone can do with being better at reading code too- traditionally people have been better at writing their own new code realtive to their ability to reading and grok their teammates code- and that doesn't scale well.

Be sure to add tasks to the sprint for the code review to happen. If it's not an actual sprint item, I find it likely won't get done effectively. If you have a couple hours, it's a great time to train the developers on some of the libraries, code standards, practices, too.

> Be sure to add tasks to the sprint for the code review to happen.

Just make sure it is a sprint and not a sprintf.

I don't understand the argument of keeping your commits small. In my team we do code review after a task is done, not for every commit so the size of the commits won't affect the review.

A commit should be like UNIX tools: should do one thing and do it well. If the commit is adding hell lots of new features, fixing bugs, and doing heavy refactoring all at once, it's way too much.

When developing say a new widget (or heavily extending an existing one), you can develop basic functionality, commit, add bells and whistles, commit, and some more stuff, commit, send for code review, do the fixes, commit, ask for re-review maybe, and then squash the commits into one commit if needed. With DVCSes like Git it's extremely easy, and reviewing smaller commits is easier than having to go through a behemoth (of course it always depends on your particular situation).

I sometimes tend to open half-baked pull requests to ask for quick early feedback if I'm doing things the right way, and what to be careful about, and continue developing in background while the more senior person takes a look at the code.

Does that really work well in practice, though? I find that I might get the base functionality down to a workable state, but then as you add the bells and whistles you're likely to find and fix your own bugs, realize you need to refactor, etc.

You can make all those changes in distinct, clean commits, but if the reviewer works commit by commit, they have to follow the evolution of your code rather than simply review the final state. You might argue that provides a basis for a better review, but it's also much more time consuming for the reviewer.

When you find an issue with previous commit while adding new things, you can make a temporary commit, fix the issue, commit once again and reorder commits, then meld the fix into the previous one. I'm pretty comfortable with doing things like that but some guys not that much. You have to like this workflow and work from the start like this.

You can of course split a huge commit into smaller ones using sth like `git add -p` but it's way more work than just doing lots of small commits regularly and then squashing them into medium-sized commits before the review. I like to do temporary commits regularly as "checkpoints", meaning, "this code works", and anytime I make a stupid mistake and break something, I can analyze a short diff to find the bug I just introduced rather than trying to figure out the bug just by looking at the code. It's usually a lot faster for me.

You don't have to push immediately to review: you commit a few commits locally, until you're sure of the architecture. Once you're sure, start pushing the earlier ones out to review.

If you're really unsure of the design, then tackle that in a design doc first. Discuss what the API will look like, write out sample call sites to show its use. Otherwise, yeah, you'll probably end up re-writing it.

Small commits have another benefit: they can be moved (cherry picked, for example to backport) or reverted easily.

When the feature is complete and ready for review, just put the final commit id in with the ticket. Compare it to some version before ticket was being worked on?

I think it's easier to look at two different versions of checkins, than to hoard code and make a single huge checkin.

It also fits with Agile: Reduce your work unit to the smallest, so you deliver a continuous stream of improvements and reduce code inventory. Code inventory (the amount of code that is created but not on prod) adds complexity (eg it adds fear of deployment) and delays the time-before-feedback. To me it's more of a good management practice than a good coding practice.

The link to codacy at the bottom doesn't work.

Thanks for heads up. Updated!

Reply on news.ycombinator.com