9 ms·

Code reviews aren’t just for catching bugs

210 points by src 10y ago

buro9 10y ago
I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review. 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".
- jaimeyap 10y ago
  This is generally why smaller changes are better. They are more easily reviewed, and more easily "steered" as you put it by a good review. 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].
- fishnchips 10y ago
  This is the role of design documents which get heavily reviewed by the team before even the first line of code is written.
  - gpderetta 10y ago
    In my experience, no design document, no matter how carefully drafted survives contact with the enemy^W^W an IDE. At best you can define interface boundaries between independently developed modules. Edit: broken leftover line
    - fishnchips 10y ago
      Things change a lot during the project, I agree. But assuming everything will change and using that as an excuse not to plan at all sounds like a poor choice to me. "In preparing for battle I have always found that plans are useless, but planning is indispensable." - Dwight D. Eisenhower
      - gpderetta 10y ago
        I do actually agree that some amount of planning, and especially putting down a sketch on paper do help, in particular as a way to nail down requirements and figuring out obvious roadstops, but it is important to be under no illusion that the final project will resemble in any way the plan
      - lmm 10y ago
        > But assuming everything will change and using that as an excuse not to plan at all sounds like a poor choice to me. I know that's how it sounds. But in my experience it turns out to be a lot more effective than planning.
  - draw_down 10y ago
    We write those on my team. They usually end up only faintly resembling what actually gets shipped. No matter how much agreement there is beforehand, as soon as people see working software they change their minds. The best thing is to plan for and expect change, not try to mitigate its manifesting.
    - fishnchips 10y ago
      True, if your software is well designed and modular then it's easy to contain necessary changes to a single subsystem. But a good design document would allow you just that - designate boundaries and interfaces between subsystems. I personally found out that writing an IDL (Protocol Buffers, Thrift or what have you) along your design document is very useful since it nicely bridges abstract concepts with actual code.
    - phasmantistes 10y ago
      Yep, and design docs are the most important part of that planning for change. If you haven't planned the desired design, you can't see how circumstances are forcing you to change it, and what tradeoffs you'll be making to accommodate those circumstances.
  - ArkyBeagle 10y ago
    Well, except for the elephant-in-the-room of the dread lord Requirements Traceability, which will frequently eat the entire design cycle.
- eric_h 10y ago
  I work on a very small team (4 engineers), so we have all commits for all branches posted to our engineering chatroom. 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]
  - dmarg 10y ago
    I work on a team with the same size as yours. Although we do not all sit in the same room, we still do code reviews. Since we are small we are not the most critical on style. 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.
    - machinshin_ 10y ago
      imo, when it comes to style, if your language ecosystem has one, you should use it to enforce one. ideally as a git pre-commit hook.
- ben_jones 10y ago
  I think it will always depend on the team. Process X might be a savior to team Y, but completely screw team A even if team A == team Y (javascript truthiness here). 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.
  - frandroid 10y ago
    > the potential downsides of excessive code review What would those be?
    - golergka 10y ago
      Imagine your team prototyping game design ideas with average speed of 1-2 days per concept.
      - frandroid 10y ago
        Prototyping ideas is not a core period for code review though... 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.
    - TheGRS 10y ago
      I'm not sure what ben_jones had in mind, but we've been doing a lot of pairing and mob programming where I work. I really enjoy it and it feels like the code review is basically just being done at all times (the reviewer is sitting right next to you as you write). But others feel like it eliminates some of the advantages of parallelism (i.e. 2 people working on 2 different things). It also slows down the amount of time to complete stories if one developer is less skilled than the other, but that issue should fade with time.
      - frandroid 10y ago
        Pair programming is an extreme version of code reviewing though :)
- wahnfrieden 10y ago
  One way to enforce this is to use source control for design docs, so that you can use the same pull request workflow as you do with code. We've done this by having user stories written in gherkin, so that new user stories can pass review, and then later can be implemented as tests. This can probably be extended to other types of design documentation.
- maldusiecle 10y ago
  How much code review you need is going to vary based on the kind of change. When my team has a large design decision, we anticipate that we're going to have to create some exploratory code that won't ever reach production. We review "spike" pull requests--maybe several generations, depending on the scope of the change. And if needed we have on and offline discussions about our alternative, the tradeoffs, etc. 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%.
- kzhahou 10y ago
  Y not both?
zhemao 10y ago
I think a benefit not mentioned in the article is that it makes sure that at least one person besides the original author understands what the code does.
- jakub_g 10y ago
  It's very important, and also it makes sure everyone is aligned about how to evolve the codebase, the project is in some coherent state and you can reason about the code as a whole (same patterns over the whole codebase etc.). 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".
- millstone 10y ago
  Unfortunately code reviews don't ensure this. Reviewers, when faced with code in a domain they don't understand, don't go out of their way to learn it; instead they just look for surface-level / nitpicky stuff.
  - thedufer 10y ago
    That's not a problem with code reviews; it's a problem with your company's implementation. "Please explain this" should be considered a very reasonable response to a code change the reviewer doesn't understand. In my experience, this ends in the code being cleaned up with readability in mind and the reviewer learning something new in more or less equal proportions.
hangonhn 10y ago
I would go further and say that you should never trust code reviews to catch bugs. They're great for all the reasons in the article and what "zhemao" said but they're horrible for catching any bugs beyond the most trivial ones. Humans simply aren't good at running code in their head like that. Code review is no substitute for good code coverage and automated testing, preferably pre-commit.
- mikestew 10y ago
  That was going to be my comment on the title. If your code reviews are "...just for catching bugs", you're doing it wrong. I put another comment in this thread describing a crappy line of code. It works (I wrote the test for it), but my...$DEITY could it have been easier to read, and made more maintainable. But it apparently sailed right through code review. 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?
- pmarreck 10y ago
  I've personally (YMMV) found that going all the way to full TDD has resulted in the fewest delivery bugs (by a lot). Beats out unit testing for sure, but either of those are miles ahead of NO testing.
- deleted 10y ago
  [deleted]
- jakub_g 10y ago
  Honest question: what strategy would you recommend to deal with a (senior) developer who specializes in opening gigantic pull request with significant number of bugs? (I invest a lot of time to read through the code and I catch lot of stuff, but it's draining huge amounts of my energy). Declining anything with coverage below 100% is not a viable option unfortunately, I think, and preaching about best practices gives little effect.
xyzzy4 10y ago
Requiring code reviews before the commit is a bureaucratic waste of time and resources. If they non-mandatory and after the commit, then they can be a good idea.
- cunac 10y ago
  wow. how so? You write perfect code and there is no need for a second pair of eyes ? This type of arrogance is always puzzling to me.
  - wellpast 10y ago
    It's not arrogance. You don't have to write perfect code to realize that the insane cost of code review is not worth the problems it is supposed to solve. Especially when code reviews are notoriously not very good anyway at excising bugs. The goal isn't perfect code. It's optimal delivery of business value. Code reviews are expensive, and not very optimal.
    - chris_va 10y ago
      Why do you find them expensive? 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.
      - wellpast 10y ago
        For me continuous integration and refactoring have been the most important practices in keeping a code base clean, robust and agile. 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.
        UK-AL 10y ago
        How hell does code review encourage those behaviours? The best commits for code review are small ones. Nobody can be bothered with the big ones.
wellpast 10y ago
What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1]. Most of the "problems" that code reviews claim to address can be solved by much more direct and optimal measures. Code reviews are damn expensive. 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.
- draw_down 10y ago
  Yes, our peer review mostly consists of looking through the diff to make sure nothing seems crazy or out of place, stray print statements, etc. Every single pull request can't go through a gauntlet of close examination, we wouldn't get anything done.
- eduren 10y ago
  >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. 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?
  - wellpast 10y ago
    Not sure how to formalize but the team's I've worked on have generally negotiated mutual respect, openness, and teamwork by collaborating on the things of greater import -- the architecture, domain conceptualization, etc. And peer code reviews in some cases tend to work against these goals--because you tend to be down in the weeds of LOC, bike-shedding, arguing over the equivalents of tabbing and spacing or whether a line could be more functionally expressed, e.g....
eduren 10y ago
Working at a Gov't IT contractor, I really wish we had the organizational skills/incentive to do code reviews. Very rarely does the code I write get glanced at, much less examined. 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.
- mikestew 10y ago
  Plenty of projects, even greenfield ones, aren't checked into source control. 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.
  - eduren 10y ago
    I agree. Source control is an integral part of my workflow and it hurts to know that if someone here were to maintain my code later, they'd just copy and paste it as-is and start from there. The reasons for this are two-fold: 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.
msoad 10y ago
Code reviews can easily become a tool for people with huge egos to prove their smartness. I get code review comments for grammar of my comments or very small code style preferences that Google's anal style guide can't enforce (yet). 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.
- chipgap98 10y ago
  That sounds like a culture problem. You don't really want people who are out to prove how smart they are, versus people that are trying to help the other engineers they work with get better.
  - deleted 10y ago
    [deleted]
  - UK-AL 10y ago
    Those two things can be presented as the same thing
- mikestew 10y ago
  And those same ones commit stuff like the LoC I'm looking at right now that returns the results of a ternary, and there's a fucking bang symbol in front of every boolean, topped off with a bang preceding the parens around that ternary (IOW, some of those bangs should cancel out, and aren't needed). I ask myself, "how did this escape code review?" and the answer is probably that the senior dev who wrote it thumped the junior dev reviewing it with something along the lines of "that's how real Java devs do it", or something. I dunno, just guessing based on personalities. Same dev that self-admits he hasn't written unit tests before, yet argues with me (who has written thousands) about how to write unit tests. 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.
BurningFrog 10y ago
Maybe I'm an arrogant XP-ist, but to me this sounds like a good step on the way to pair programming.
- johnrob 10y ago
  Depending on the org structure, pair programming can sometimes be a clearly superior version of a code review policy. Particularly when the review latency is high and, as a consequence, developers work on several branches in parallel.
- Cthulhu_ 10y ago
  It is, but with some differences; works better for remote teams (see github), is async (don't need to interrupt someone to get cracking), is more suitable to introverts (or anyone really, who can pair program for 8 hours / day or more?), and you get to sit down and review someone's code with a fresh pair of eyes instead of not catching mistakes because you wrote them or were there while they were being written.
gracenotes 10y ago
As several other commenters have pointed out, code reviews are not a silver bullet. It is widely known that there is no silver bullet. 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.
qaq 10y ago
You can often have 100% test coverage and critical change code reviews, for about same cost as doing code reviews for each commit.
- renku 10y ago
  It's hard to decide which are the critical changes. Even a single line change can have major consequences - where do you draw the line... 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.
  - qaq 10y ago
    Obviously depends on a project and team size. In a reasonably sized team a team lead would be the one to draw the line.
raldi 10y ago
Code reviews : good programming habits :: sexual reproduction : good evolutionary traits They make them spread much much faster than they otherwise would.
maxxxxx 10y ago
How do people handle reviews of highly specialized stuff? We have people who do stuff nobody else on the team understands or at least it would take them a long time of learning to do a real review. I look at a lot of stuff and check if it makes halfways sense. I can look at the coding style but I can't judge the overall design without spending many hours on it (which I don't have. Nobody else on the team has it either). I am starting to think that pair programming may use up less engineering time than doing thorough reviews.
- nradov 10y ago
  That's a management failure. The organization can't depend on a single developer being the only one to understand a module. What happens when that developer leaves, even temporarily? A competent manager will dedicate time to cross training so that at least one other team member understands everything, even if this causes a short-term productivity loss.
  - maxxxxx 10y ago
    That sounds good but in our case that's really not feasible. If a certain developer leaves almost anybody in the team can take over his work in a few weeks. But nobody has the team to stay up to date all the time. We would have to staff up quite a bit. This would be nice but it ain't gonna happen.
- neandrake 10y ago
  Where I work the same situation often comes up -- a developer may spend weeks doing research for a specialized function building prototypes, etc. 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.
danielweber 10y ago
At a software shop about ten years ago I loved code reviews. They were a major way of learning and teaching things. I stopped doing some silly (albiet harmless) habits when other people pointed them out.
aczerepinski 10y ago
I'm a dev with one year's experience, and recently moved to a team that reviews every PR. The benefits to me personally are super clear. #1, more experienced engineers are giving me frequent feedback, and that's obviously worth a ton. #2, it's part of my job to read other developers' code. I get exposed to patterns and design choices that I may not have in my repertoire. 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.
- alxndr 10y ago
  This is a great attitude to have!
wandernotlost 10y ago
While I value many of the same things as the author, I've found code reviews to be far inferior in every respect to pairing (especially promiscuous pairing (google it)), and to have negative effects in several important ways: * 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. Of these, the most egregious is the distraction from value. Teams that are spending a lot of time and energy talking about code quality (which is absolutely important, but not primary...the best teams I've seen maintain a high level of quality and talk about tradeoffs involved in delivering value at high quality) are often neglecting communication about where value lies and how to deliver it most efficiently. [edited for formatting]
- db1 10y ago
  Interesting, this is the first time I'm hearing about promiscuous pairing. How big the team at your company is and what percentage of time is spent pair programming?
  - wandernotlost 10y ago
    I do consulting, so I work with a lot of different teams of various sizes and configurations. The best teams tend to pair close to 100% of the time and generally have 4-10 developers. Fewer than that makes it difficult to swap around, mix perspective, and keep things fresh, and more than that starts to introduce more complications in planning and coordinating work streams and sharing context.
- Sacho 10y ago
  I agree that those things could be issues with code reviews, but I don't see how "promiscuous pairing" helps with most of them. * 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.
mirekrusin 10y ago
In small/medium teams, especially remote ones, code review helps you keep understanding of the system - you already know what your code does and it's great way to learn the rest of the system. In some sense, it can be looked at, as asynchronous pair programming.
harwoodleon 10y ago
What about the stress of continuous evaluation? How does the company view that? Standards are great to maintain, but people aren't robots. I question the value of the word 'review' in this context and its blocking 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.
V-2 10y ago
"not JUST for catching bugs"? I always thought catching bugs is the least of their purposes
voidr 10y ago
Code reviews: * 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.
- onion2k 10y ago
  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 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.
  - profmonocle 10y ago
    It's not just programming. Journalists and authors have editors for this exact reason.
- reacweb 10y ago
  My boss performs code reviews before my work is delivered. In my case it is mostly in a positive way: - he wants to know how stuff is implemented because he may need to maintain it if I am on leave. - it is a motivation for me to produce good code because I know it will be read - he may give good advice (for example usage of ArrayList instead of Vector in java)
- aavz 10y ago
  Assuming that people will make mistakes writing code isn't distrust, it's just common sense. 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.
goda90 10y ago
My employer develops safety critical software with decades of legacy code, long term support for multiple versions, and big customers. In our attempts to transition(in a mock way for the time being) to rolling releases we've "simplified" to a process with 3-5 people reviewing designs, 2-3 developers doing code review/light testing, 2-x(depending on affected modules) quality assurance people doing heavy testing and occasionally we pair program. Documentation/logistics is almost always more time consuming than design and development. I've had 4 character code changes take weeks to get through the process as several people have to find the time to look at it even for just an hour. It's cumbersome but we do catch a lot of bugs, and we're slowly getting better at design.
waigani 10y ago
This is a topic close to my heart. I’m a software engineer for Canonical (company behind Ubuntu). Last year I did a study of the total review wait time over a two month period, which came out to be 8683hrs ( http://lingo.reviews/d3/juju_review_wait_times.html http://lingo.reviews/d3/juju_review_wait_times.html). 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 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

210 points by src 10y ago

buro9 10y ago

I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review. 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".

jaimeyap 10y ago
This is generally why smaller changes are better. They are more easily reviewed, and more easily "steered" as you put it by a good review. 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].
fishnchips 10y ago
This is the role of design documents which get heavily reviewed by the team before even the first line of code is written.
- gpderetta 10y ago
  In my experience, no design document, no matter how carefully drafted survives contact with the enemy^W^W an IDE. At best you can define interface boundaries between independently developed modules. Edit: broken leftover line
  - fishnchips 10y ago
    Things change a lot during the project, I agree. But assuming everything will change and using that as an excuse not to plan at all sounds like a poor choice to me. "In preparing for battle I have always found that plans are useless, but planning is indispensable." - Dwight D. Eisenhower
    - gpderetta 10y ago
      I do actually agree that some amount of planning, and especially putting down a sketch on paper do help, in particular as a way to nail down requirements and figuring out obvious roadstops, but it is important to be under no illusion that the final project will resemble in any way the plan
    - lmm 10y ago
      > But assuming everything will change and using that as an excuse not to plan at all sounds like a poor choice to me. I know that's how it sounds. But in my experience it turns out to be a lot more effective than planning.
- draw_down 10y ago
  We write those on my team. They usually end up only faintly resembling what actually gets shipped. No matter how much agreement there is beforehand, as soon as people see working software they change their minds. The best thing is to plan for and expect change, not try to mitigate its manifesting.
  - fishnchips 10y ago
    True, if your software is well designed and modular then it's easy to contain necessary changes to a single subsystem. But a good design document would allow you just that - designate boundaries and interfaces between subsystems. I personally found out that writing an IDL (Protocol Buffers, Thrift or what have you) along your design document is very useful since it nicely bridges abstract concepts with actual code.
  - phasmantistes 10y ago
    Yep, and design docs are the most important part of that planning for change. If you haven't planned the desired design, you can't see how circumstances are forcing you to change it, and what tradeoffs you'll be making to accommodate those circumstances.
- ArkyBeagle 10y ago
  Well, except for the elephant-in-the-room of the dread lord Requirements Traceability, which will frequently eat the entire design cycle.
eric_h 10y ago
I work on a very small team (4 engineers), so we have all commits for all branches posted to our engineering chatroom. 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]
- dmarg 10y ago
  I work on a team with the same size as yours. Although we do not all sit in the same room, we still do code reviews. Since we are small we are not the most critical on style. 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.
  - machinshin_ 10y ago
    imo, when it comes to style, if your language ecosystem has one, you should use it to enforce one. ideally as a git pre-commit hook.
ben_jones 10y ago
I think it will always depend on the team. Process X might be a savior to team Y, but completely screw team A even if team A == team Y (javascript truthiness here). 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.
- frandroid 10y ago
  > the potential downsides of excessive code review What would those be?
  - golergka 10y ago
    Imagine your team prototyping game design ideas with average speed of 1-2 days per concept.
    - frandroid 10y ago
      Prototyping ideas is not a core period for code review though... 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.
  - TheGRS 10y ago
    I'm not sure what ben_jones had in mind, but we've been doing a lot of pairing and mob programming where I work. I really enjoy it and it feels like the code review is basically just being done at all times (the reviewer is sitting right next to you as you write). But others feel like it eliminates some of the advantages of parallelism (i.e. 2 people working on 2 different things). It also slows down the amount of time to complete stories if one developer is less skilled than the other, but that issue should fade with time.
    - frandroid 10y ago
      Pair programming is an extreme version of code reviewing though :)
wahnfrieden 10y ago
One way to enforce this is to use source control for design docs, so that you can use the same pull request workflow as you do with code. We've done this by having user stories written in gherkin, so that new user stories can pass review, and then later can be implemented as tests. This can probably be extended to other types of design documentation.
maldusiecle 10y ago
How much code review you need is going to vary based on the kind of change. When my team has a large design decision, we anticipate that we're going to have to create some exploratory code that won't ever reach production. We review "spike" pull requests--maybe several generations, depending on the scope of the change. And if needed we have on and offline discussions about our alternative, the tradeoffs, etc. 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%.
kzhahou 10y ago
Y not both?

zhemao 10y ago

I think a benefit not mentioned in the article is that it makes sure that at least one person besides the original author understands what the code does.

jakub_g 10y ago
It's very important, and also it makes sure everyone is aligned about how to evolve the codebase, the project is in some coherent state and you can reason about the code as a whole (same patterns over the whole codebase etc.). 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".
millstone 10y ago
Unfortunately code reviews don't ensure this. Reviewers, when faced with code in a domain they don't understand, don't go out of their way to learn it; instead they just look for surface-level / nitpicky stuff.
- thedufer 10y ago
  That's not a problem with code reviews; it's a problem with your company's implementation. "Please explain this" should be considered a very reasonable response to a code change the reviewer doesn't understand. In my experience, this ends in the code being cleaned up with readability in mind and the reviewer learning something new in more or less equal proportions.

hangonhn 10y ago

I would go further and say that you should never trust code reviews to catch bugs. They're great for all the reasons in the article and what "zhemao" said but they're horrible for catching any bugs beyond the most trivial ones. Humans simply aren't good at running code in their head like that. Code review is no substitute for good code coverage and automated testing, preferably pre-commit.

mikestew 10y ago
That was going to be my comment on the title. If your code reviews are "...just for catching bugs", you're doing it wrong. I put another comment in this thread describing a crappy line of code. It works (I wrote the test for it), but my...$DEITY could it have been easier to read, and made more maintainable. But it apparently sailed right through code review. 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?
pmarreck 10y ago
I've personally (YMMV) found that going all the way to full TDD has resulted in the fewest delivery bugs (by a lot). Beats out unit testing for sure, but either of those are miles ahead of NO testing.
deleted 10y ago
[deleted]
jakub_g 10y ago
Honest question: what strategy would you recommend to deal with a (senior) developer who specializes in opening gigantic pull request with significant number of bugs? (I invest a lot of time to read through the code and I catch lot of stuff, but it's draining huge amounts of my energy). Declining anything with coverage below 100% is not a viable option unfortunately, I think, and preaching about best practices gives little effect.

xyzzy4 10y ago

Requiring code reviews before the commit is a bureaucratic waste of time and resources. If they non-mandatory and after the commit, then they can be a good idea.

cunac 10y ago
wow. how so? You write perfect code and there is no need for a second pair of eyes ? This type of arrogance is always puzzling to me.
- wellpast 10y ago
  It's not arrogance. You don't have to write perfect code to realize that the insane cost of code review is not worth the problems it is supposed to solve. Especially when code reviews are notoriously not very good anyway at excising bugs. The goal isn't perfect code. It's optimal delivery of business value. Code reviews are expensive, and not very optimal.
  - chris_va 10y ago
    Why do you find them expensive? 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.
    - wellpast 10y ago
      For me continuous integration and refactoring have been the most important practices in keeping a code base clean, robust and agile. 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.
      - UK-AL 10y ago
        How hell does code review encourage those behaviours? The best commits for code review are small ones. Nobody can be bothered with the big ones.

wellpast 10y ago

What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1]. Most of the "problems" that code reviews claim to address can be solved by much more direct and optimal measures. Code reviews are damn expensive. 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.

draw_down 10y ago
Yes, our peer review mostly consists of looking through the diff to make sure nothing seems crazy or out of place, stray print statements, etc. Every single pull request can't go through a gauntlet of close examination, we wouldn't get anything done.
eduren 10y ago
>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. 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?
- wellpast 10y ago
  Not sure how to formalize but the team's I've worked on have generally negotiated mutual respect, openness, and teamwork by collaborating on the things of greater import -- the architecture, domain conceptualization, etc. And peer code reviews in some cases tend to work against these goals--because you tend to be down in the weeds of LOC, bike-shedding, arguing over the equivalents of tabbing and spacing or whether a line could be more functionally expressed, e.g....

eduren 10y ago

Working at a Gov't IT contractor, I really wish we had the organizational skills/incentive to do code reviews. Very rarely does the code I write get glanced at, much less examined. 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.

mikestew 10y ago
Plenty of projects, even greenfield ones, aren't checked into source control. 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.
- eduren 10y ago
  I agree. Source control is an integral part of my workflow and it hurts to know that if someone here were to maintain my code later, they'd just copy and paste it as-is and start from there. The reasons for this are two-fold: 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.

msoad 10y ago

Code reviews can easily become a tool for people with huge egos to prove their smartness. I get code review comments for grammar of my comments or very small code style preferences that Google's anal style guide can't enforce (yet). 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.

chipgap98 10y ago
That sounds like a culture problem. You don't really want people who are out to prove how smart they are, versus people that are trying to help the other engineers they work with get better.
- deleted 10y ago
  [deleted]
- UK-AL 10y ago
  Those two things can be presented as the same thing
mikestew 10y ago
And those same ones commit stuff like the LoC I'm looking at right now that returns the results of a ternary, and there's a fucking bang symbol in front of every boolean, topped off with a bang preceding the parens around that ternary (IOW, some of those bangs should cancel out, and aren't needed). I ask myself, "how did this escape code review?" and the answer is probably that the senior dev who wrote it thumped the junior dev reviewing it with something along the lines of "that's how real Java devs do it", or something. I dunno, just guessing based on personalities. Same dev that self-admits he hasn't written unit tests before, yet argues with me (who has written thousands) about how to write unit tests. 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.

BurningFrog 10y ago

Maybe I'm an arrogant XP-ist, but to me this sounds like a good step on the way to pair programming.

johnrob 10y ago
Depending on the org structure, pair programming can sometimes be a clearly superior version of a code review policy. Particularly when the review latency is high and, as a consequence, developers work on several branches in parallel.
Cthulhu_ 10y ago
It is, but with some differences; works better for remote teams (see github), is async (don't need to interrupt someone to get cracking), is more suitable to introverts (or anyone really, who can pair program for 8 hours / day or more?), and you get to sit down and review someone's code with a fresh pair of eyes instead of not catching mistakes because you wrote them or were there while they were being written.

gracenotes 10y ago

As several other commenters have pointed out, code reviews are not a silver bullet. It is widely known that there is no silver bullet. 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.

qaq 10y ago

You can often have 100% test coverage and critical change code reviews, for about same cost as doing code reviews for each commit.

renku 10y ago
It's hard to decide which are the critical changes. Even a single line change can have major consequences - where do you draw the line... 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.
- qaq 10y ago
  Obviously depends on a project and team size. In a reasonably sized team a team lead would be the one to draw the line.

raldi 10y ago

Code reviews : good programming habits :: sexual reproduction : good evolutionary traits They make them spread much much faster than they otherwise would.

maxxxxx 10y ago

How do people handle reviews of highly specialized stuff? We have people who do stuff nobody else on the team understands or at least it would take them a long time of learning to do a real review. I look at a lot of stuff and check if it makes halfways sense. I can look at the coding style but I can't judge the overall design without spending many hours on it (which I don't have. Nobody else on the team has it either). I am starting to think that pair programming may use up less engineering time than doing thorough reviews.

nradov 10y ago
That's a management failure. The organization can't depend on a single developer being the only one to understand a module. What happens when that developer leaves, even temporarily? A competent manager will dedicate time to cross training so that at least one other team member understands everything, even if this causes a short-term productivity loss.
- maxxxxx 10y ago
  That sounds good but in our case that's really not feasible. If a certain developer leaves almost anybody in the team can take over his work in a few weeks. But nobody has the team to stay up to date all the time. We would have to staff up quite a bit. This would be nice but it ain't gonna happen.
neandrake 10y ago
Where I work the same situation often comes up -- a developer may spend weeks doing research for a specialized function building prototypes, etc. 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.

danielweber 10y ago

At a software shop about ten years ago I loved code reviews. They were a major way of learning and teaching things. I stopped doing some silly (albiet harmless) habits when other people pointed them out.

aczerepinski 10y ago

I'm a dev with one year's experience, and recently moved to a team that reviews every PR. The benefits to me personally are super clear. #1, more experienced engineers are giving me frequent feedback, and that's obviously worth a ton. #2, it's part of my job to read other developers' code. I get exposed to patterns and design choices that I may not have in my repertoire. 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.

alxndr 10y ago
This is a great attitude to have!

wandernotlost 10y ago

While I value many of the same things as the author, I've found code reviews to be far inferior in every respect to pairing (especially promiscuous pairing (google it)), and to have negative effects in several important ways: * 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. Of these, the most egregious is the distraction from value. Teams that are spending a lot of time and energy talking about code quality (which is absolutely important, but not primary...the best teams I've seen maintain a high level of quality and talk about tradeoffs involved in delivering value at high quality) are often neglecting communication about where value lies and how to deliver it most efficiently. [edited for formatting]

db1 10y ago
Interesting, this is the first time I'm hearing about promiscuous pairing. How big the team at your company is and what percentage of time is spent pair programming?
- wandernotlost 10y ago
  I do consulting, so I work with a lot of different teams of various sizes and configurations. The best teams tend to pair close to 100% of the time and generally have 4-10 developers. Fewer than that makes it difficult to swap around, mix perspective, and keep things fresh, and more than that starts to introduce more complications in planning and coordinating work streams and sharing context.
Sacho 10y ago
I agree that those things could be issues with code reviews, but I don't see how "promiscuous pairing" helps with most of them. * 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.

mirekrusin 10y ago

In small/medium teams, especially remote ones, code review helps you keep understanding of the system - you already know what your code does and it's great way to learn the rest of the system. In some sense, it can be looked at, as asynchronous pair programming.

harwoodleon 10y ago

What about the stress of continuous evaluation? How does the company view that? Standards are great to maintain, but people aren't robots. I question the value of the word 'review' in this context and its blocking 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.

V-2 10y ago

"not JUST for catching bugs"? I always thought catching bugs is the least of their purposes

voidr 10y ago

Code reviews: * 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.

onion2k 10y ago
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 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.
- profmonocle 10y ago
  It's not just programming. Journalists and authors have editors for this exact reason.
reacweb 10y ago
My boss performs code reviews before my work is delivered. In my case it is mostly in a positive way: - he wants to know how stuff is implemented because he may need to maintain it if I am on leave. - it is a motivation for me to produce good code because I know it will be read - he may give good advice (for example usage of ArrayList instead of Vector in java)
aavz 10y ago
Assuming that people will make mistakes writing code isn't distrust, it's just common sense. 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.

goda90 10y ago

My employer develops safety critical software with decades of legacy code, long term support for multiple versions, and big customers. In our attempts to transition(in a mock way for the time being) to rolling releases we've "simplified" to a process with 3-5 people reviewing designs, 2-3 developers doing code review/light testing, 2-x(depending on affected modules) quality assurance people doing heavy testing and occasionally we pair program. Documentation/logistics is almost always more time consuming than design and development. I've had 4 character code changes take weeks to get through the process as several people have to find the time to look at it even for just an hour. It's cumbersome but we do catch a lot of bugs, and we're slowly getting better at design.

waigani 10y ago

This is a topic close to my heart. I’m a software engineer for Canonical (company behind Ubuntu). Last year I did a study of the total review wait time over a two month period, which came out to be 8683hrs ( http://lingo.reviews/d3/juju_review_wait_times.html http://lingo.reviews/d3/juju_review_wait_times.html). 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 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