Code reviews aren’t just for catching bugs
blog.fullstory.com
blog.fullstory.com
I prefer the idea of a code review happening at a time that it could still steer the ship... too many code reviews catch bugs, but don't correct poor system design. That's also where the greatest benefit/education exists for the author... not minor changes, but "what about approaching it like this".
And of course having a design doc ahead of time that the team can review and comment on for a longer, more complicated change, is a great thing to do. And compliments the subsequent code review[s].
Edit: broken leftover line
"In preparing for battle I have always found that plans are useless, but planning is indispensable." - Dwight D. Eisenhower
I know that's how it sounds. But in my experience it turns out to be a lot more effective than planning.
Everyone knows what everyone else is working on (and we all work in the same room), so when someone is on a task that we know my have some snags/difficult-to-design solutions, we'll all periodically take a break and look at decisions other people are making on their particular feature, providing feedback when appropriate.
Of course, since we all work in the same room, we can also use the WTF/minute metric to see (well, hear) when one of us is deep in the weeds and could use a second pair of eyes to pull us out.
[EDIT: and I know this won't scale too far beyond a team of our size, but for us it works quite well right now]
I am a big believer in code reviews especially starting early with a small team. This would set the culture from the beginning because it is harder to bring that in later.
On the flip side I think you can also say the potential downsides of excessive code review are significantly less then the potential downsides of zero code review.
I dunno.
What would those be?
ETA: If you end up using a prototype, then you clean up the code at the end of the 1-2 days, and request a review at that point. Code reviews are not meant to get in the way of development. They're a post-development step that happens before QA/testing.
https://twitter.com/girayozil/status/306836785739210752
Not to mention trivial commits, like clarifying comments, something which doesn't happen anymore in my current project - nobody can be bothered to go through the review process for that.
I'm not against code reviews, but I prefer casual post-commit reviews for uncontroversial changes.
Sometimes that works out fine, but other times you don't find out until too late that there are major flaws.
I think this works a lot better than trying to impose design reviews on every change. Most changes shouldn't require this kind of scrutiny; you don't want to slow down 95% of code reviews for the sake of the difficult 5%.
This post concedes that code reviews are better for the more fluffy ends -- teamwork, openness, social recognition, but given their high costs, I'd rather achieve even these soft goals in other ways than to impede my team's delivery potential.
While mission critical systems deserve the whole kitchen sink thrown at them, expensive verifications, code reviews, etc etc., most business applications would do much better optimizing for better software architectures and domain conceptualization than spend so much time dwelling on the minutiae of lines of code.
[1] Continuous integration and refactoring, pillars of agility, go out the window in typical code review environments where commits are blocked until peer review.
What techniques have you found effective for improving the soft goals in the context of software (genuinely curious)? Are we talking more conventional management/business concepts or something a bit more loosely defined?
In the absence of a strong style guide and testing culture then I agree code reviews can get bogged down in bikeshedding. At Google that doesn't seem to happen because the style guide for each language is quite prescriptive. For Go, no change will be reviewed that hasn't passed through gofmt, and so forth. And on the flip side you can have confidence that if your change has gone through gofmt, there's not going to be any discussion about the formatting.
Another huge cost of code reviews is distraction. We've all seen the Paul Graham essay on maker's schedules vs. manager's schedules. We've all read the statistics on how much time is lost to interruptions. Code reviews are a massive interruption, done on a manager's schedule. Each code review is a distraction, and can take a significant time commitment, if it is to be a meaningful review.
A few years ago, I worked at a company with a strong emphasis on code reviews, and it turned into a waste of time when people couldn't afford to waste time. You'd have to do the review, but you really needed to get back to your development or bug-fixing because of the impending hard deadline, so there would be non-commital non-review reviews such as "looks good to me", or "I see no problems". While a good code review can be valuable, these perfunctory ones are a huge waste of time.
Finally, code reviews sometimes substitute for design review, which catches the most serious problems much earlier. At this same company, it drove me crazy that we always had time for code reviews, but never for design reviews.
Wait why are we doing code reviews on the manager's schedule? Why not just wait until you're finished a task to do them?
You reminded me of one cargo-scrum team working like that.
Important design decisions were made thoughtlessly on "sprint planning" meetings since they were required for division of the "story" into "tasks". Then the classic "garbage in, garbage out" law guided the sprint.
This is just my opinion of the matter. On the topic of team building, I don't really care for all that stuff. Unit tests w/ CI and periodic refactoring are great for revisiting design decisions. Code reviews are great for catching bad design decisions early on, though so use it as a tool.
In my experience code reviews are much cheaper than other means of raising software quality. For example unit tests only start to add real value after you have written a good bunch of them, so they form this regression-safety-net.
I haven't seen Continuous integration and refactoring being thrown out of the window because of code reviews. Quite the opposite. Code reviews usually ask the author to refactor his code further. Similarly I have no idea why one would stop continuous integration because of code reviews.
Code reviews mean that every time I want to integrate with master I've got to 'grab a lock' and schedule and wait for a code review.
What typically happens in these code review cultures is you tend to make monolithic commits (to minimize the # of code reviews you have to do) and you also tend to minimize diffs with the code base (to make the CR easier for the reviewer).
Code reviews mean that every time I want to integrate with master I've got to 'grab a lock' and schedule and wait for a code review.
You're using a terrible code review system. You should be able to have multiple potential commits out for review at any given time. You should also have multiple pending commits from your coworkers on your queue at any given time. The code review process is asyncrhonous: process them at your leisure (though sooner is better than later, for obvious reasons). Once the review has been approved, it can be rebased onto or merged into master. If the patches you're writing are so far-flung that they don't apply cleanly, break them into smaller pieces.
What typically happens in these code review cultures is you tend to make monolithic commits (to minimize the # of code reviews you have to do) and you also tend to minimize diffs with the code base (to make the CR easier for the reviewer).
These two statements are contradictory. Monolithic commits by definition increase the diff with the codebase, not decrease it. What you tend to see are meaningful commits: commits which do one specific thing, and fully test it, without breaking any other systems.
The idea of code reviews is that somebody else reads your code and provides feedback. Whether the reviewer is sent a pull request, a list of commits in master or a printout is a matter of implementation.
I see a cyclic problem in here: Reviews take lot of time -> developers want to get more done, not wait for a review -> so they send more changes to review -> reviews take even more time -> ...
I think the solution is to find ways to make reviews smaller instead. Breaking tasks to smaller pieces that take less time to implement and review. (But not by reducing the # of lines in diff - squeezing several changes into one commit is a sure way to make it harder to review.) Smaller reviews will also be more effective - with large reviews there's a tendency for a reviewer to wear off and just skim through the changes.
Of course that's easier to say than do... it's something I'm trying to do by myself and not fully succeeding.
Separately, minimizing diffs with the codebase is not just easier for the reviewer, it also makes 'blame' and resolving problems with existing code easier. You shouldn't break your back to minimize diffs, but as a form of hygiene it is not without merit.
Your definition of continuous integration is interesting in that I haven't seen it used in that way, or rather with that emphasis. Smaller syncs-to-master than "this is the entire feature" are great, but their effectiveness depends on automated testing, building, and deploying. When code under review is a coherent, high-quality, and tested thing, this can be a boon for CI because you get a small chunk with clear before and after states, which you might not get from just winging it. You can also work on code while other code is under review, especially with a Scrum/Kanban-type task system.
Without it the rate of change in the branch was fast enough that submissions would be preempted by someone else's and you'd have to rerun presubmission tests etc.
I'm on a team where for about a year, part of our process has been that code doesn't get merged until three people say it's ready. If one person wrote it, it needs two reviews, or if a pair wrote it, it needs one review.
Your mileage may vary, but my experience with this is that the code reviews increase latency, but don't really have much effect on bandwidth. It might mean that there's a few hours between finishing your code and merging it into master, but during those hours you can be doing other things. Most code reviews take <10 minutes, and I usually just check for other people's PRs when right after I create a PR of my own, so it doesn't interrupt my flow.
Sometimes PRs do get blocked, but in those cases it's usually for good reasons; the design has some serious flaw or is very overcomplicated, or there's a bug. I see that as similar to a failing build. Sure, a CI server sometimes blocks you from merging, but do you really want something that fails tests or doesn't compile going into your master branch anyway? Code review is just like having a CI server that builds and runs tests, but it checks things that can't be automated.
We do sometimes run into cases where code review takes a long time, but the root cause there is usually that we didn't break down the feature into an adequately small enough minimum viable product. 1000 lines of changes takes more than 10 times as long to review as 100 lines of changes; the increase isn't linear. If code review is taking a long time, it may be an indication that your continuous integration isn't continuous enough.
That said, "code review is 100% necessary" and "code review is 100% too costly" are just different kinds of Kool-Aid you shouldn't drink. "People and interactions over processes and tools"[1] applies here. What works for my team might not work for yours. I'm not arguing for code review; there's no universal right way to create software. I'm arguing that code review is an effective process step for some teams and an ineffective one for others.
* Understanding of the problem at hand, reading through the story, understanding the context of the the code change.
* Pulling down the code, reading through the commit log
* Reading the specs, possibly running coverage tools if not part of testing suite.
* Understanding the logic of the code, seeing if the tests cover the edge cases
* If dependencies change, they may need to be investigated too
* Thinking if there are better ways make code a bit more extensible or understandable
* Thinking of risk of the code (security, performance, deployment concerns)
* Reading through associated documentation and ensuring its accuracy
Doing all of this takes me longer then 10 minutes always.
Also, some code reviews are tiny changes, so it brings the average down.
* We do our code reviews on GitHub by looking at the diffs. No need to pull code.
* We review the code of the tests, but we don't run them. Our CI server runs tests and coverage tools.
* The code is usually a short MVP, <100 lines including tests, so understanding logic and edge cases doesn't take long.
* If dependencies change, it was a group discussion and we already investigated.
* Again, small commits make this quick.
* Small commits make this quick.
* Documentation is handled by our product manager.
I guess what I'm learning from reading your comment is that part of why our code reviews are fast compared to yours is that we mean drastically different things when we say "code review".
2) Reviews can be done privately, with comments entered electronically. There doesn't have to be a four-day meeting.
2a) Organizations that use four-day meetings are likely full of people who need something to charge that time to. It may be worth delegating a representative from the engineering team to attend those.
3) Action items must be triaged. Ideally, each action item would be assigned a number and numbers grouped together as a statement of work for a commit. This is much less painful than it sounds. Test provenance would also be nice per commit.
I don't get why you think code reviews are so expensive. An engineer should rarely be blocked by a code review. If waiting for a review - don't. Pick up another task! A great thing about code reviews is they are done async.
If the cost is time spent doing the review - what's the alternative? No code review at all? That's like not testing - yes it's faster now, but you'll pay for it dearly later on. (Code review is well studied in academia / industry, and has consistently shown to be very effective. Having at least one extra pair of eyeballs on any code goes a long way.)
Also, a lot depends on how code reviews are done. I've seen them used as a veto, with lots of back and forth to get the veto removed over subjective and trivial issues. Don't do that.
Finally, let's be honest. Some people's code doesn't need review. Time spent reviewing such code is wasted. But it is often socially difficult to say this, so everyone's code gets reviewed.
Other people can't be trusted to write acceptable code, and reviews are essential. That's a different problem. (Beyond the scope of this note. Left as an exercise to the reader. Other cliches may apply.)
wat?
Either the code is easily readable and correct, and takes barely any time at all to review, or it's buggy and detecting this early rather than late saves a yuuuuge amount of time.
Also, the reviewer now knows the code. So in reviewing the code they've already half way to being able to improve the code in the future.
And the reviewer can also pick up "oh, that's a neat pattern". Code reviews are education for both parties. Are you saying companies should not spend time or money on education/courses?
And as an author I really appreciate not only that someone looks for me having made a mistake, but because I know someone will read the code I won't go with the easy way. I can fool myself, but I can't fool someone else. E.g. the right thing to do is to name this constant or make an enum, but I'll just put a literal "4" here because I know it's 4. (which of course I won't remember in a week).
* They delay integration.
* They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code.
* They favor superficial improvements, while increasing the costs sunk into paths that may be deeply flawed. (They facilitate late code-structural feedback, but not early directional/problem-analytical feedback.)
* They often discourage more collaborative work and therefore quicker and richer feedback, by their presence as a substitute for pairing.
* They create impediments to work moving quickly to completion.
[edited for formatting]
* They delay integration.
Pairing by definition slows down all work on code(you have 1 person working instead of two). Also, code reviews need not delay integration, even if you're following the article's suggestions. You can still merge all work in an integration branch, and pull it if it eventually doesn't pass code review.
* They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code.
This is an issue with the priorities of the people doing the code review, and thus applies just as much to pairing.
* They favor superficial improvements, while increasing the costs sunk into paths that may be deeply flawed. (They facilitate late code-structural feedback, but not early directional/problem-analytical feedback.)
I have no idea how pairing is possibly different from code review in this case.
* They often discourage more collaborative work and therefore quicker and richer feedback, by their presence as a substitute for pairing.
This is possibly true, so I'll just take the assertion at face value.
* They create impediments to work moving quickly to completion.
This is pretty much the same as #1.
"Of these, the most egregious is the distraction from value." - so the most egregious problem with code reviews is the people that do them might have wrong priorities. I don't see how pairing would change this.
A benefit of pairing that I can see over code reviews is when you get feedback - instant, real-time in pairing vs late in code reviews. The useful scenario I envision is when you start implementing a feature and your pair-buddy steers you away from dumping time into a poor solution. That and #4 seem to be significant reasons why you might want to introduce pair programming. However, pair programming is not without drawbacks when compared to code reviews - it takes up more time in most cases(this is where it derives its main benefit), some people work better solo, a mismatch of skill or expertise means one person has to slow down to work with the other. I wouldn't really say code reviews are "far inferior in every respect" to pairing, I think both have their place.
> I don't see how "promiscuous pairing" helps with most of them.
Maybe you should try it! The "promiscuous" part transfers context and knowledge around more quickly, and gives more opportunities, earlier in the process, to respond to feedback from a wider variety of perspectives. Two people might come to the same conclusion that another, joining a couple of hours later and without following the same mental path, would find crazy. It's generally pretty easy to change direction after a couple of hours.
> Pairing by definition slows down all work on code(you have 1 person working instead of two).
"Work on code" is not all that's involved in shipping product. In my experience, pairing produces less/simpler/better code that more directly addresses intended business value and cuts off unproductive paths more quickly than other methods. I much much much prefer cranking out less of the right code than producing tons of "perfect" code that solves the wrong problem (slightly hyperbolic, but you get the point, and it totally happens all the time). It's worth mentioning that what I'm talking about is pairing done well, by people who have learned how to do it well. It's absolutely possible to do a shit job pairing, as with anything else.
>* They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code. >This is an issue with the priorities of the people doing the code review, and thus applies just as much to pairing.
This is true to a certain degree, but good pairing typically involves thousands of little tradeoff decisions and discussions about how to solve problems. A code review can't reproduce the richness of all of that communication, so it necessarily emphasizes code in the abstract over deep consideration of the tradeoffs involved in the solution to a problem.
> I have no idea how pairing is possibly different from code review in [the case of superficial improvements.]
When I'm sitting down with someone to solve a problem and we explore a path for a few minutes, then I can step back and say, "wait, based on what we've just done, I think we're thinking about this whole thing in the wrong way, what about this other thing," it's easy to turn around and explore a totally different approach. When somebody has sunk an entire day into solving a problem, polishing the code to impress their coworkers, and responding to code review comments, it's extremely difficult to say, "hey, I think we're thinking about this the wrong way, what about this other thing?" I've experienced this problem nearly every time I've participated in code reviews and rarely during pairing. (To that point, pairing is such a better dynamic for this type of exchange. A code review has an oppositional nature, while with pairing, you're literally on the same side of the problem, sitting together to find a solution. The dynamic of questioning the path/approach starts off in a much more natural/friendly/collaborative place.)
> so the most egregious problem with code reviews is the people that do them might have wrong priorities
I think it goes beyond that. As mentioned above, the structure of code reviews (vs. pairing) favors certain types of feedback over others, and makes the feedback that I believe to be most valuable very difficult to give, socially, and too late to be really valuable.
>A benefit of pairing that I can see over code reviews is when you get feedback - instant, real-time in pairing vs late in code reviews.
Yes! Exactly. And from what I've seen, this makes an enormous amount of difference.
I don't agree with you about the time differences, especially when considering the full cycle of delivery. It's a notoriously difficult thing to measure effectively and/or prove, so...it's one of those things you have to experience first hand (in an environment where it's done well...there are plenty where it's not) to really buy into perhaps.
It's also true that some people just want to work solo. I'm motivated by producing the best possible product and generating the best possible outcomes, and pairing is the best way I've encountered to do that, so...in general, I'd prefer not to work with those people when they're steadfastly opposed to trying collaborative work. At the same time, I've found that most people who give it a shot with a good pair who has some patience, empathy, and skill, end up loving it. Even folks who generally work alone.
I like code reviews, don't get me wrong. But there should be a way to respond with "you just shut up, you're only trying to make yourself look smart".
It's all because higher up people mostly look at code review conversations this is happening. The other day I asked my peer how do I write this code? A or B? He said I don't care, A seems fine. Then in code review he commented it should be done in B way.
It's all politics.
Though I wonder if what you describe isn't just plain ol' bike shedding. I've seen it plenty in code reviews. Super sharp dev who generally writes great code puts a commit up for review, and someone feels like they ought to have some input, and because the code is otherwise solid they pick on grammar.
Slightly clearer comments isn't a bad thing? Why not fix it in the two seconds it takes to.
No one's going to repremanded or promoted on code reviews.
Im a senior dev(ops) and don't mind if I get code review full of really minor issues by junior devs try to prove themselves. Every little issue fixed, makes a it a little bit better.
$foo=fn1($bar) &&$baz = fn2 ($qux)
I figure code reviews are for reviewing stuff that can't be corrected by a computer.
If you can't write a coherent sentence or consider a style guide 'anal', you have no place developing for a project that is serious enough to warrant code reviews.
Another thing, if someone submits a PR with basic errors like that, they distract from the things that should be looked for in a code review - bugs, implementation details, etc.
Like you, I'd say that code review isn't for catching bugs at all. Is it readable? Can some Shmoe off the street who is hired to maintain make sense of it? How's the complexity? Do I need to maintain eight different states in my head to grok it? Et. al. Now, a team can help prevent potential bugs (either outright as the code stands, or bugs introduced later when someone tries to maintain it and it's a pasta derivative) with code reviews. I mean, didn't we run the unit tests we wrote before we submitted it for code review? Why are we finding bugs just by looking at the code?
And then continue to reject it until you get nicely chunked up stuff.
This means it decouples all the components which leads to check-ins or pull requests that are also self contained and modular.
If you push for good test coverage (less than 100% but more than 80% perhaps?) for your unit tests, you'll get less bugs and more modular code and smaller check-ins.
I view bugs as a potential issue in an individual's development process. It sounds like the biggest reason you already hit on -- gigantic pull requests. Gigantic requests usually mean PRs that aren't focused to one discrete work-piece, but represent work that kinda meanders to completion. Make sure they're focused, and ask for nonessential pieces to be delegated to another PR (style-only PRs being a good example).
Ultimately the only way to suppress large pull requests (or problematic practices in general) is to evangelize simple development workflows that others can help you with. Keep in mind that change for anyone, no matter intelligence or stubbornness, takes time - culture especially. You need to make it easier for them to adopt your methods instead of theirs when the time comes and they get fed up and are ready to change. Make sure you don't have an antagonistic relationship, either, or they'll do anything but what you want just to spite you :(
I've broken up some stuff to look for in code reviews, in code bases, and in developers. You probably only want to look at developers.
For the code itself:
- Do we have overdeveloped patterns that get in the way of expressive code? - Do these bugs fall into a general classification that can pinpoint a source (such as timing issues, authentication, database calls, or info validation?) - Is your code-base DRY? Repetitious functionality sprinkled around will mean bugs that never get fixed. Eliminate repetition. - Age of code-base? Newer code-bases should be more accepting of large PR's/more bugs, but should compensate with refactoring-only PRs, test blitzes, and accessible code coverage. Old code may have traditions that have outlived their usefulness, and owners that have grown too accustomed to the state of the code to understand the need for improvement. - No experts. Is this an inherited code-base with poorly documented patterns whose original authors have moved on?
For code reviews (speeding up review):
- First read-through of the code is only to familiarize myself with the code added. - Does this PR solve one problem or a bunch of problems? One feature/bug/style-change == one PR (per repository). Ask to break it up if it's big and doing more than one thing (unless those things are highly coupled). - Is there core functionality? Start with the most reused piece, and critique that first. - Is this code shared? Shared code should be held to a higher standard, especially if other people depend on it. Get their eyes on it too, it takes some work off you, builds ownership, and if this person is a problem, you'll want allies to back you up. - Are methods long? Longer methods tend to do more than one thing, and invites glomming onto existing methods instead of creating your own. Higher standards for shared code modification tends to lead to shorter methods, because reusing code needs to be a deliberate decision. Plus long methods can hide code reuse. - Is manual testing difficult? Long or difficult verification loops tend to allow for more bugs.
For the developer:
- What are their tools? What tools are you using that makes you more effective? Learning new tools is hard, but most bugs come from ineffective development practices - Are they senior in knowledge or senior in age/time? Not all senior devs deserve their title. - Do they stay on-task? Do they have to thrash between tasks? Unfocused development is a big source of bugs. - How familiar are they with the code base? Senior or not, code from new devs should require more scrutiny (but not to the point that it discourages collaboration).
Also with code reviews you make sure other devs are not (re)introducing some bugs that have been fixed before (maybe in other places of the app), and so on. There can be objective technical bugs, but code review will catch also the misconceptions about how stuff is supposed to work in the app, which can have profound consequences - no matter how great your code is technically, it you're building a square while circle was requested, it's not good.
I'm absolutely stunned by the comments in this thread. I've worked for a short while in a team with no code reviews, everyone was pushing whatever the hell they wanted and it was a nightmare (and git history was a total spaghetti). Soon after I joined the pull request workflow was introduced but it was too late, the code was so bad it was unmanagable.
Unless you have really magnificent team, chance are too high that someone will be writing non-maintainable/non-debuggable code, using super confusing variables/methods/classes names, or reinventing the wheel, or not handling errors properly. "We don't do code reviews" is a no-go for me at this point, second to only "We use ClearCase for version control".
Maybe the feedback I give the other direction isn't yet as valuable as what I receive on my PRs, but I trust that eventually it will be.
I totally get the posts pointing out that during crunch time, folks do pretend reviews, and the process becomes busy work. I think that's a symptom of other problems though (staffing model, etc), and not necessarily a shortcoming of peer review in general.
That said, if your goal is to make a quality product, you shouldn't have to choose between code reviews and continuous integration. You shouldn't have to choose between code reviews and code coverage, or manual QA processes. These are all widely regarded as best practices and if implemented "correctly" and appropriately to the team their combination forms a virtuous cycle for code health and team culture.
They make them spread much much faster than they otherwise would.
My instinct, of course, is to solicit code reviews from my peers, but the organizational structure and support tooling are all woefully inadequate. Plenty of projects, even greenfield ones, aren't checked into source control. And of course if I spent time contacting those in my org that have similar skills and could thus review, not only would I have to justify it to my 3-4 managers, but so would the person I solicited for review to their own.
Nobody cares about the code quality, and thus it has been a nightmare for me trying to improve my skills right out of school. Here's to hoping I get out soon.
I really and truly did not know this still happens. Hell, even on throwaway/PoC stuff for which I am the sole developer, and code that stands a good chance of never seeing the light of day, I start with git init. 'cuz the probability that I'm going to wish later that it was in source control outweighs the very minor cost of putting it in there. For an organization that produces software that others will later use, I'm at a loss to explain it other than inertia.
1) Like you said: Inertia. Most projects/developers here have been around for years, many starting before git was a thing. SVN is around and used quite a bit, but like I said, I've talked to developers that use neither. Since the code lives on the server, and the server is backed up, people feel no need for source control. Which is part of...
2) At least in the web development I'm doing, there's little to no "collaboration". I have been the sole person actually writing code on every project so far. There are teams working together, but usually every project has a single developer. This place is so vulnerable to their developers getting hit by a bus. But management doesn't care because 12 months down the line, once the developer is no longer working the project for whatever reason, the project is scrapped and either re-done because no one else was involved in the technical details, or they spend another year in federal procurement hell to get a shitty off-the-shelf product.
Most projects/developers here have been around for years
I just made a comment this morning another dev that it occurred to me that the first program I ever wrote was compiled 40 years ago (digression: the reason I brought it up was to ask, "so why the hell do I still make off-by-one errors?"). The period of which I've used source control can be measured in decades. Until recently, the only reason I didn't use source control on a project was because back in the day SCM cost money, money that wasn't always available.
many starting before git was a thing
But, and I'm sure you're well aware so bear with me, source control has been around long before git showed up. The difference now is that git (and CVS and SVN before it) is free, as in beer, speech, whatever. That leaves us with little excuse these days (other than git being a general pain-in-the-ass to use).
Again, I'm not disagreeing with you personally. I guess it really just turned into a rant against lazy devs and/or the glacial organizations for which they work.
Saying that, poor code quality has been a real incentive for me to improve my own code. When I eventually work out what something is doing and see how much simpler it could be i try to redo it in a way that will be a lot more understandable to the next person. If I visit my own code a month or two later and don't immediately understand it, its time to refactor it. At the end of the day I prefer writing code to debugging it, so the less time I have to spend trying to understand code, the less time I spend debugging.
Code quality that simply reduces bugs and speeds maintenance is a lot less important than the kind of code quality that creates architecturally sound solutions and good products. Where I work, the lack of quality control/assurance affects so many things and has a visible detriment to the products.
I am starting to think that pair programming may use up less engineering time than doing thorough reviews.
Ensure the specialized developer is doing due-diligence in defining and verifying the module works as intended and have others analyze its interactions in the larger system to prevent cascading failure, conforms to application norms, __is documented__, etc. Even if most others wouldn't understand the internals during review you will be taking steps to keep the risk localized. If the bus arrives and you lose the specialized developer then it's reasonable to assume another developer would need to spend nearly as much time to catch up on the research.
Otherwise you're responsible for making the decision to invest more developer time now in understanding the problem/implementation vs. later - a gamble at how much time until that developer leaves.
Taking over specialized/legacy code is part of the job. It can be painful but there are steps you can take to minimize that pain later.
Documentation is indeed the name of the game. Few things are more frustrating than seeing a code do something and having no idea why.
I want to see comments, names of any clever algorithms used, links to applicable datasheets, research papers, etc. A short overview of the whole design is nice, too.
I wrote up my thoughts around scalable engineering here: bit.ly/1P0YgNo and released an MVP solution here: www.lingo.reviews
Since then I’ve been refining privately with a handful of engineers from different companies. Two days ago I put in my notice and took on solving the problem of scalable engineering as a full-time mission: http://codelingo.io
I'd love to connect with anyone that is passionate about this problem (it's been a 2 year obsession for me): jesse@codelingo.io
* signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted
* can create a culture of passive aggressiveness - you do something that I don't like, I'll get back at you when I'll review your code
* not guaranty code quality - having a junior review a junior's code will not yield expert level code
Just because Google does code reviews doesn't mean you should, for Google a bug could cost millions, for your project a bug might cost 10$, however the overhead of code reviews might cost more than 100$. It's important to do numbers and think rationally.
That doesn't have to be a bad thing. You (and everyone else) can't be trusted to be totally infallible, so if you want to produce good work relying on many eyes to catch mistakes, suggest improvements, or to learn from one another then you need to look at everyone's code. A code review should be a conducted in a safe, blame-free environment where everyone involved wants to make better software. That's should be the goal, not finger pointing or points scoring.
If someone is a good developer, I trust their first cut of code will be well thought out. I don't assume that they considered all corner cases or found the best design or written things in a way that makes sense to other people on the first try.
Things go downhill if people start taking code reviews personally, but then I don't think the real problem is code reviews.
Are you then assuming that the person who will review the code will consider everything?
I distrust me by default, and you should too. Humans cannot write correct code, and the way to keep high code quality is to maximise our ability to fix the errors that result from having humans involved. I don't want me submitting any code that hasn't been looked at by another person. While I do (almost every day) manage to write some CLs that get approved without comments, I definitely have many CLs every day where somebody will say "This is confusing" or "There's no test for this part" or "Here's a better idea that I had".
If your team members are engaging in passive-aggressive abuse then you should find new ones, not try to do your job without interacting with them.
A person of the same experience level as me will routinely find things that I missed, just because they didn't spend two hours writing the code and are taking a fresh look at it. The same thing is true of a person more junior than me, if we can make them not be shy and write comments like "I don't understand what this does, therefore it is too confusing". No reviewer guarantees code quality, because nothing guarantees code quality, but my experience is consistently that 1 reviewer is a massive improvement over 0 reviewers, with marginal improvement based on reviewer experience.
The true cost of not doing code review is that your code will be harder to maintain in future, giving continual costs for its entire lifespan. The only code I consider to have a cost/benefit ratio that makes it worth skipping the review phase is code that I don't intend to keep for very long.
This kind of breeds a dilemma: If humans don't know how to write correct code, why are we trusting them to verify code correctness? :)
No matter how many times I run jslint, I always get the same result, however if I would show the same code to 20 programmers, I'm pretty sure I would get a lot of different results.
> If your team members are engaging in passive-aggressive abuse then you should find new ones, not try to do your job without interacting with them.
If your friend looks at your code, (s)he'll find a lot less issues than your rival. We software engineers are not emotionless objective beings.
My issue is that I have seen few issues that could have been caught in time by code reviews with the cost of the code review being less than the cost of just fixing the problem. Code reviewing every change is continuos effort, that might cost more than having a few quirks and fixing it.
I do understand some projects do require every kind of verification process you can throw at it, like software that controls nuclear power plants, however not everyone is building that kind of software.
The reason I don't trust these methods is because they seem to ignore human nature.
Pair programming is a far more effective tool, but it's not always practical. How about using the terminology of 'collaboration' instead of the test based culture - which I feel turns people into machines - it's just the wrong control structure for people to be happy.
In some sense, it can be looked at, as asynchronous pair programming.
The goal isn't perfect code. It's optimal delivery of business value. Code reviews are expensive, and not very optimal.
I've done thousands in my career. They don't take much time compared to actually writing the code, and adding an extra 5% of engineering time pays major dividends later without drastically reducing throughput.
Code review cultures tend to encourage the opposite of these agile practices: monolithic commits, infrequent integration, and minimal diffs -- in other words, practices that tend to result in lesser productivity.
The best commits for code review are small ones. Nobody can be bothered with the big ones.
Once you have all that, the purpose of a code review changes a bit, but it is still really useful. Maybe you don't need locking, or maybe you should rewrite the base class, or maybe there is some other part of the code base that should be folded in, or maybe a better interface/algorithm. Stuff like that is hard for a computer to detect for you, at least for the next decade or two :).
The really big thing I push in code reviews is defensive programming. I want to make sure someone in a 2 years, who isn't familiar with the code, will not screw up the codebase after the original authors may have moved on to other things.
When this happens, a common response is to go work on something else, but this only exacerbates the problem: now you have a bunch of commits awaiting review which are dependent in various ways. If I merge this commit I have to go and fix up that one, and that other one. My work becomes quadratic, nursing a bunch of commits along while waiting their turn through a tiny review pipeline. 5% can be 100%.
Things can be just as bad if code is not written well.
Doing either well requires good engineering practices and skill. It is part of the profession that you acquire over time, like anything else that can improve development.
Now, sometimes you are at the mercy of a system not filled with really experienced engineers. In such a system, though, I think skipping code reviews for expediency will probably be even worse in the long run.
I agree with you, even in the ideal case of a co-located homogeneous team, it's possible to screw up the process.
I guess both 100% code review coverage and 100% test coverage are extremes that one shouldn't worry too much about. But my suspicion is that 100% test coverage can do more harm than reviewing of each commit.