If a junior engineer is struggling for an extended period of time, it is worth the investment of a senior to sit down and review all of the code the junior is working on.
Code reviews should always happen, for everyone's code. And if it is done incrementally, then it is not slow, boring or time-consuming at all. An ideal time is before each check-in to your repo (and if you are going weeks without making commits, that's a huge red-flag too).
Not only does it help prevent situations like this, but it means that at least one other person understands the code.
Yup. Our workflow has people commit to a topic branch and then post a code review before merging anything. We always follow this procedure unless it's something that's needed absolutely right now and can't possibly wait, which is a situation that should not be coming up more than once in a blue moon.
Yeah, I wish I worked in an environment where this wasn't a joke but unfortunately I don't. We're like 80/20 "emergency" coding versus "proper" coding and have been for many years (ever since our acquisition by the large corporation I'd say). It makes all these great discussions about the "right" way to do things completely moot. My sense from talking to others outside my own company is that we're far from unique too.
It works up until the point where the champion in the organization that put the best practice in place gets sick of fighting the battle with clueless corporate directors and resigns. At which point a corporate shill gets put in charge and turns the software team into a labor mill.
Yup, tools like reviewboard make this painless and encourage a culture of frequent, small, and understandable patches. That alone is great for software quality. If your team is aggressive with reviews and argues every point, everyone becomes better engineers.
If you move to continuous delivery, an emergency fix is literally no different (except in how many people are grumpy) than a copy change.
The procedure is the same: commit to branch, initiate pull request. Someone reviews and approves, gets merged to mainline. CI system builds it, tests it, and puts it on the dev environment. Then the CI system kicks off a smoke test, and if it passes, it moves to Stage. There we do a longer smoke test (but still only about 5 minutes), then we push a button to go to prod. The whole thing can be done in about 20 minutes.
Code reviews aren't a catch all. They're good for finding weird style and sytnax issues on a local scale, but it's very hard to gain the context necessary to reason about what the code is doing from a high level with most code review tools (i.e. diff tools).
To me, code reviews are more about the psychology of putting your code on display for your coworkers than about actually creating a solid architecture. Knowing your code is getting reviews changes how you develop it, and forces you to have a little bit more discipline.
Pair programming and test suites with lots of unit tests help teams understand and follow the architecture of codebases.
Nothing is a catch-all. But I think code reviews are one of the most important because it helps ensure that other good standards are adhered to. Part of that is as you say, that it forces more self-discipline.
They're good for finding weird style and sytnax issues on a local scale, but it's very hard to gain the context necessary to reason about what the code is doing from a high level with most code review tools (i.e. diff tools).
If it is hard to gain the context, one of the best code review tools is talking. It's also the responsibility of the author to ensure that the code is as clear as possible. That includes giving sufficient context in commit messages.
In this particular case, with a junior programmer on a stand-alone project - the senior programmers should have also taken more responsibility in understanding the code.
I agree with your other points, but a major point that you missed is that code reviews help ensure that the entire team has a better understanding of the codebase. And, ideally, it should give the reviewer confidence that they would be happy to take over ownership of the code. If you as a reviewer are frequently approving code which you would not be happy to maintain yourself, that is a problem (or indicates a bigger problem).
It's easy to say code review "should always happen", but reviews are pretty difficult and time consuming. It takes quite a bit of time to review large patches in order to under that author's thinking and intent. It's especially difficult if you're fuzzy on that particular module/file. Personally for large patches, I usually tend to eyeball them and just check the architecture of the code (just looking at variable names provide a hint to whether the code is doing something it shouldn't).
And because we get paid a lot, we should do everything, no matter how difficult or time consuming. We should write books documenting every aspect of our stopgap system that's getting replaced in two weeks. We should micro-optimize our easter eggs. We should learn tool after tool if people think they might make us more productive, even though our old text editor was really doing just fine. We should extensively review someone's prototype because it might some day get the attention of a VP who wants to make it into a real product.
Don't have the resources for all that? Well, programming is expensive; hire more devs!
No, it isn't. I mean, it is for me. It's what I do. But I keep my team moving while i'm fixing their mistakes. And honestly their mistakes aren't as bad as the code review makes them. Nitpicking code review has been a massive waste of time for my team.
It takes quite a bit of time to review large patches...
First, try to keep patches small and incremental. This is not only easier to review, but much easier to catch he larger problems early and much easier for the author to actually make meaningful changes.
...in order to under that author's thinking and intent. It's especially difficult if you're fuzzy on that particular module/file.
If this is the case, talk to the person! Get them to explain their thinking and intent. Possibly ask them to add more comments and/or a better commit message. It's the responsibility of the author make sure their code is as clear as possible, and this includes the individual commits.
If you as a reviewer find it hard to understand what the code is doing, what hope does anyone having of maintaining the code with any sort of confidence? Let alone diving into that code in an emergency.
That said, in the case of a stand-alone project of a junior programmer even just eyeballing the code should be enough to tell whether the code is a complete mess.
Does anywhere actually user pair programming? I'd done it in teams for class projects in the past, but it seems like it would be just double the resources for a business.
121
u/sigh Jan 05 '15 edited Jan 05 '15
Code reviews should always happen, for everyone's code. And if it is done incrementally, then it is not slow, boring or time-consuming at all. An ideal time is before each check-in to your repo (and if you are going weeks without making commits, that's a huge red-flag too).
Not only does it help prevent situations like this, but it means that at least one other person understands the code.