[HN Gopher] No code reviews by default
___________________________________________________________________
No code reviews by default
Author : signa11
Score : 88 points
Date : 2022-01-04 09:16 UTC (13 hours ago)
(HTM) web link (www.raycast.com)
(TXT) w3m dump (www.raycast.com)
| eyelidlessness wrote:
| In my experience, code reviews promote trust. They make changes a
| collaboration and a conversation, where reviewers and
| implementor(s) bring different intuitions and concerns, and help
| expand understanding of risks and goals. Sure, a lot of that can
| happen before any code is written, but it's inherently more
| abstract.
|
| Code reviews aren't just about code, and probably shouldn't be
| framed that way.
| blank_dan wrote:
| If your code is unimportant then so are your bugs that get into
| production. So I guess I agree. Don't waste time writing,
| testing, reviewing unimportant code.
| DaveEM wrote:
| This is an interesting read and I'm glad to see that the closing
| paragraphs emphasize the need for each team to evaluate their own
| needs. That said, for me it all comes down to the definition of
| "best product" in this statement:
|
| "They all want to build the best product in the shortest time
| possible"
|
| From my perspective, the "product" is not limited to what
| customers see or the code itself. It's the set of outcomes from
| the code being written and executed. For example... Do other
| engineers understand the code? Can they support it efficiently?
| Does it fail gracefully? What is the impact of failures? Are
| failures automatically detected? Does it lead to security
| breaches? Did the code author learn things they could do better
| that they didn't know to ask about? Did other engineers learn
| from reading the code or asking questions? Are customer
| requirements satisfied? How much is customers' trust impacted by
| failures, bugs, and feature that don't meet requirements? How big
| is your cloud provider bill? Etc...
|
| If the priority is quickly delivering and iterating on features
| that satisfy customers' needs AND engineers will be supporting
| their own code, then the "No Code Review by Default" approach has
| its merits. In fact, I have personally found this approach to
| work well on constrained, isolated pilot projects where the
| priority is to unblock key scenarios for a limited set of
| customers and/or learn about customer needs before building a the
| "real" solution.
|
| For anything else, I would suggest following the author's own
| advice: "ask yourself if the circumstances of others apply to
| you".
| revskill wrote:
| Code review in real world means peer review, it's the PEER part
| that matters, not the CODE part.
|
| I do think collaboration matters here, not about trust or code.
|
| Good software comes from good collaboration.
| alkonaut wrote:
| Sounds nice but perhaps a best fit if you are 1) a small team 2)
| people know and trust each other already 3) people are all and
| equally comfortable asking for reviews (disturbing others,
| essentially). 4) the code has a priority on feature progress
| rather than stability and correctness.
|
| The situation can be very different in any of those aspects and
| mandatory review can still be a good idea. Are you building code
| that runs on surgery equipment together with 4 people you just
| met? I think you should probably have a formal review process. Or
| are you iterating towards an MVP for some webapp with a few
| people you consider friends? Lgtm just deploy to prod.
| integrate-this wrote:
| This is a great way to end up in a situation where you have to
| re-write entire modules whenever you need to update old code...
| which is kinda where our teams tend to end up anyway so I guess
| it's not _that_ awful?
| brokencode wrote:
| I think this would work really well for a small and experienced
| team, though where I work, we have a lot of developers fresh out
| of college, and I would be pretty nervous setting them loose
| without close review of their work.
| skeeter2020 wrote:
| Especially because a HUGE benefit of code reviews is exposure
| and knowledge transfer. For us it's a key area where anyone can
| get insight into the how and why, plus control their
| participation from requester/reviewer to passive observer. It's
| actually a pretty poor place to catch bugs introduced by new,
| junior devs.
| klabb3 wrote:
| The idea is not that everyone has to be omnipotent, it's that
| you trust people enough to send for review and ask for help
| _when appropriate_ , instead of always. I.e. when you're new to
| the code base or language.
| brokencode wrote:
| I don't think I've ever reviewed code from a new hire that
| didn't have multiple problems or style issues that needed to
| be addressed. That's also actually true for code from most
| experienced devs too. And it's rare for my code to get
| through code review with no issues found. So bottom line, I
| just don't think this would work for most teams and
| developers.
| ReleaseCandidat wrote:
| > I just don't think this would work for most teams and
| developers.
|
| Most teams and developers don't do code reviews because the
| only person that could do code reviews would be the
| developer himself.
| brokencode wrote:
| Do you mean to say that most developers work alone, so
| don't have anybody to review their code? That's probably
| true for hobby projects and the like, but companies
| rarely have teams of one working on anything.
| PeterWhittaker wrote:
| We're very small (3 full time devs, a mostly full time tester, a
| part time student, and another student temporarily full time),
| and the three full timers are pretty seasoned, so this may not
| apply universally, but....
|
| We don't review code as a matter of course. But if anyone of us
| writes code that doesn't pass our personal smell test, or that we
| think it won't pass someone else's smell test, or <insert
| criteria here>, screens are being shared and ideas tossed about.
|
| We almost always <solve the problem we were having|create a
| better solution|decide to accept the debt and open an issue until
| we have a better way>, in pretty short order.
|
| So it's sort of JIT/OnDemand code reviews. Kinda.
| Puts wrote:
| This can not be posted too many times.
|
| https://www.youtube.com/watch?v=4XpnKHJAok8
|
| Linus Torvalds talk on Google about how GIT is more a way of
| working then a piece of software. Everybody commits upwards in a
| tree of trust. If you do it this way you get automatic code
| reviews and in any team someone should be responsible for the
| "product" anyway and highest up in the hierarchy.
| jeffbee wrote:
| It's a great example of how Linus has a massive blind spot for
| the failings of the git model. He's "sorry" they use Perforce.
| Git is "better than everything else out there" but he doesn't
| mention Perforce. And yet, the Perforce model is the one that
| got Google to be one of the fastest-moving organizations on the
| planet, with the biggest code base.
| [deleted]
| nix0n wrote:
| > the Perforce model
|
| I've used both Git and SVN enough to understand how a VCS
| might change how an organization creates code, but I've never
| used Perforce. What is "the Perforce model"?
| jeffbee wrote:
| In Perforce the concepts are different. You have a client,
| with a view that includes a subset of the repo, and you
| have changelists which incorporates your related edits.
| When done with a changelist, you submit it to trunk,
| usually, because while you can branch a Perforce repo, you
| mostly don't need to.
|
| Basically all the stuff in the first half of the talk where
| he whines about stepping on toes, conflicting with other
| people, politics, special write access groups, are all non-
| issues that do not sound familiar to Perforce users. If two
| people are using Perforce they can both work on changes to
| the same files taken from the main branch (trunk) and
| submit them separately without conflict or even awareness
| of the other person. As a bonus this is all dramatically
| faster in Perforce than in git for large repos.
|
| In this talk Linus demonstrates that he believes git is a
| state-of-the-art SCM system, without being familiar with
| the actual state of the art.
| tus666 wrote:
| He mentions "trust" a lot.
|
| Is that why we do code reviews, because we don't trust each
| other? I find this attitude problematic. We do code reviews
| because humans make mistakes. Requirements can be misinterpreted.
| Different work in progress can be in conflict with each other.
| Reviews are a good way to learn from each other and keep abreast
| of what work is occurring outside your own bubble. Not doing code
| reviews because of "trust" kind of misses the point.
|
| I find even just cursory and brief code reviews are beneficial
| and often pick up on issues. It also creates a more collaborative
| team and gives me a reason to communicate with devs I might not
| otherwise communicate with, even if that is just a brief comment
| on a PR.
|
| Rejected.
| alecbz wrote:
| Yeah, IMO this is like a publisher saying "we don't have
| editors: we trust our authors". The intended ethos of editors
| and code review is that mistakes and imperfections are the
| norm, and you need a second pair of eyes to iron them out.
|
| That said, I do sometimes encounter individuals or cultures
| that seem to view code review more as a mechanism for catching
| abnormal/unexpected mistakes than as a normal part of the
| process of writing code.
| globalise83 wrote:
| Trust but verify
| snarf21 wrote:
| I think it is trust in the trust and verify sense. Meaning, we
| trust each other to do our best but let us verify that we are
| all on the same page and aren't breaking things or doing
| something harmful.
| sanderjd wrote:
| Yes, catching mistakes (which everyone makes) and bidirectional
| communication are the points of code reviews, it's not a lack
| of trust.
| ljm wrote:
| If something hit production and caused a major fuck-up because
| there was no peer review process, then in all the places I've
| worked at the first action item in the post-mortem would be "we
| should introduce peer review." Otherwise someone would ask how
| we could ensure it wouldn't happen again, and no one would be
| able to say "because we trust them," and leave it at that. It
| would sound more like "I trust you will never make that mistake
| again," which sounds more like a threat. You also can't go to
| your clients and say "sorry, we let our team deploy stuff
| without reviewing because we trust them... so please continue
| to deal with our avoidable problems."
|
| Thing is, I've lead my fair share of teams and I trust my team-
| mates and colleagues implicitly. It makes for a strong team.
| But we still do code review (peer review) because we want to
| build software that works well and support each other. We're
| not in the job to simply deploy code to prod as quickly as
| possible.
|
| I'd also add that I think any software engineer should try and
| have the experience of working in a highly regulated field,
| like healthcare. It's hard to get an appreciation for why these
| things exist until you realise you're held to a much higher
| level of accountability because your oopsie moments can have
| much larger consequences. For me, it's been hard to go back to
| my old, cavalier attitude after that.
| jolux wrote:
| Causing a major fuck-up in production is probably also a sign
| that you need better release validation and deployment
| practices.
| jaredsohn wrote:
| For many developers, people merge their branches directly
| to production so the pull request review is where that
| checking happens.
| jolux wrote:
| Sure, I mostly meant automated processes. They won't
| always save you from yourself but doing blue-green,
| having an extensive test suite, etc, are all things that
| will help reduce the risk of a bad deployment.
| watwut wrote:
| Ok, but many companies don't use such ridiculous process.
| kcb wrote:
| It's not as ridiculous as it sounds as long as the
| testing and verification happens as part of the PR.
| dtech wrote:
| iirc Google does it, it's not ridiculous at all. You need
| a lot of automated tests and canary deployment to pull it
| off though.
| alkonaut wrote:
| Exactly. I want my code peer reviewed partly _because_ it
| makes it clear and formal that while we succeed as a team we
| also fail as a team. It's much easier to talk about failures
| when a failure doesn't have a single person attached to it.
| rdw wrote:
| This post sounds like wisdom, but after looking at it hard,
| it seems that the point is no more than "peer review
| processes are popular". Not to say that code review is bad in
| any way (I do it at my current role), but, this argument for
| them is not very good.
|
| One downside of code review is reduced velocity. There are
| teams out there who use code review so effectively that they
| never ship big fuck-ups to production. They never have to
| tell clients that they have bad practices. Instead, they
| struggle to get clients because they pay much more per line
| of code shipped and therefore don't deliver as good of a
| value as their competitors.
|
| This where the argument needs to be: whether and how to have
| code reviews be a net positive.
| ljm wrote:
| Ultimately I think it's down to where you want to pay for
| that decision (for want of a better term), or where to put
| the bottleneck. I think that putting the bottleneck in
| production (which would block all work unrelated to a fix
| if a catastrophic fuckup took place) might as well be a
| hedge against the possibility of things fucking up so badly
| in a given timeframe. You could also make code review a
| bottleneck, but I haven't seen that work well in practice
| compared to putting it at QA or as part of a release
| process (unless you combine them in a continuous delivery
| workflow).
|
| I can see how that equation balances in favour of no code
| review when it's in the context of a startup that is
| probably still figuring out how to make money, especially
| with a subscription based app launcher. As far as
| prototyping and early stage iterations go, you're probably
| just seeing what works and reviewing the code isn't so
| important at that point. There would still be one or two
| things you'd ask someone to check, e.g. security related
| stuff.
|
| I think you make a good point though: what makes an
| effective code review process? I don't think there's a
| single answer to that because it depends on the
| circumstances.
|
| I would at least say that it goes better if it's treated as
| a priority and issues are raised and dealt with promptly,
| rather than leaving it as a chore you eventually get around
| to. And you would have to see value in it beyond it being a
| sanity check, as others in this thread have described
| (knowledge transfer, for example).
| [deleted]
| watwut wrote:
| The best code I have seen was on projects without code review.
| And projects with it were more mess - they were surface
| consistent but overall hard to comprehend.
|
| The deciding factor was ownership and accountability tho - you
| maintained own code and if you done it crappily, you knew.
|
| The code review is related to assumption that everyone can
| change everything - meaning all in all inconsistent mess. It is
| also related to unfortunate assumption that code review is how
| people learn system. They don't, unless they are original
| author. The rest of then knows pieces of it only.
| sanderjd wrote:
| This is the exact opposite of my experience.
| devanl wrote:
| I agree that code review is not a great medium to disseminate
| information about the codebase, but assigning individual
| ownership and accountability doesn't make the need to share
| information go away.
|
| Ownership and accountability are workable as long as the same
| person sticks around long enough to fix all their mistakes.
| If that person leaves, the code becomes orphaned and it's up
| to one of their teammates to find out where the skeletons are
| buried.
| watwut wrote:
| No it does not. But it also leads to one-two clear person
| you know to ask. It also leads to same person explaining
| same thing multiple times. So as long as he cares just a
| little, the explanation will improve.
|
| And the person doing explanation actually understand the
| whole part. That is big one too - you are not explained
| bits of it by someone who knows only small parts of it. And
| the system was not changed by third party without the
| person who explains having at least vague idea it was
| changed.
|
| > Ownership and accountability are workable as long as the
| same person sticks around long enough to fix all their
| mistakes. If that person leaves, the code becomes orphaned
| and it's up to one of their teammates to find out where the
| skeletons are buried.
|
| That is actually exactly the same without clear areas.
| Except everyone is new all the time.
|
| If you have high turnover, this aspect will be bad matter
| what.
|
| And it is not that bad with responsibility either. Old
| person does explains things to new person. Taking over,
| when you are working consistently in same area is not that
| hard, actually. Some parts you conclude to be mess, but
| usually not whole of it.
|
| ---------
|
| And imo, quality and knowledge heavily depends on other
| parts of team. Analysis and testing. Both, if work
| reasonably well are massive resource for knowing how the
| hell the system is supposed to work.
|
| And once you have reliable source for requirements, that is
| not developers, then deciphering the code is much easier.
| devanl wrote:
| In my mind at least, the issue with relying on on
| ownership, especially in a smaller team with limited
| resources, is that the owner isn't just the person who
| authoritatively understands that subsystem. The owner
| ends up being assigned _all_ of the work on that
| subsystem because it's "their responsibility".
|
| As a result, any work that's assigned to someone else
| will only interact with at the interfaces / boundaries.
| And of course, that's sort of the intention with
| modularity / loose coupling - it's not necessary to learn
| the gory details of the XYZ subsystem's implementation,
| only its API. Knowing the details is certainly valuable,
| but there's no baseline impetus to get the owner to
| explain the details.
|
| Taking over from someone who's ceding responsibility, but
| staying at the company, I agree that's manageable. But
| sometimes people get laid off suddenly (or perhaps get
| hit by a bus, etc) and there's no chance to get the one
| person who has it all in their head to explain it to you.
|
| You mentioned high turnover, but if anything, a place
| with high turnover has less information locked up in a
| single person's head. If Alice who's worked at the
| company for 25 years and has always owned the code
| leaves, you're going to have a much harder time getting
| all of the tribal knowledge out of her head on two weeks
| notice compared to Bob who only made it 6 months.
|
| To me, the value of code review is that even at its
| worst, someone other than the author is forced to look at
| the code. No process can force a reviewer to take an
| active interest in a particular subsystem, but at least
| we can make them look at pieces of it and see if it gets
| them curious enough to ask further questions of the
| author and better understand the system.
|
| For myself, knowing that someone else will have to
| understand my code, even through the limited lens of a
| code review, makes me more diligent about making sure
| that the code is clean and the pull request has enough
| context that someone playing code archeologist will be
| able to make sense of it, even if I'm not walking them
| through it in real time. I will admit that this is not a
| universal thing - I've worked with coworkers where no
| matter what process is in place, they won't do the basic
| courtesy of reading their own changelists to see that
| they accidentally committed random junk files from their
| local machine.
|
| I agree that good documentation around requirements is
| valuable. I find pull requests / code reviews are a great
| opportunity to highlight how the code relates to specific
| requirements, since the focus is narrower.
| marcosdumay wrote:
| A formal review step where you accept or reject the code, yes,
| is always there because of lack of trust.
|
| Informal reviews can have a lot of different shapes, and
| provide a lot of different benefits. But anything that is a
| stop-the-process activity is there because you expect quality
| problems from the earlier steps.
|
| (That said, no good developer trusts oneself. So if you don't
| have anything on your process that makes you trustworthy, lack
| of trust is the correct approach.)
| xmlninja wrote:
| I trust you with my life but do I also trust you with my money
| and my wife?
| actually_a_dog wrote:
| Absolutely. I take issue with all 3 of his main points. You
| covered the first point well, so, I'll take the last 2.
|
| > Pull requests don't prevent bugs. Reviewing code is hard! You
| can catch obvious mistakes, but you won't have consistent in-
| depth scrutiny. Many issues reveal themselves only after
| extensive use, which doesn't happen during code reviews. We
| rely on fast iterations with user feedback.
|
| So many things I can say about this. "Fast iterations with user
| feedback" are great, until you introduce a security bug that
| you find out about because someone exploits it. Catching
| "obvious" mistakes alone has significant value. Since he
| acknowledges that code reviews catch _some_ bugs, this
| criticism seems to be that they don 't catch _all_ bugs. To
| that, I say: show me a practice which _does_ catch all bugs,
| and I 'll show you why it doesn't. Even the most rigorous
| development process doesn't prevent _all_ bugs. [0] This just
| doesn 't sound like a fair criticism to me.
|
| > Pull requests slow down. Code reviews aren't high priority
| for engineers. They have to make time to clean up their review
| queue. In reality, PRs are an afterthought and not what you
| want to spend your time on as an engineer.
|
| This is largely a culture problem. Code review _should_ be a
| high priority for engineers. Maybe it 's not the _highest_
| priority, but, it 's fair to expect one could get a decent
| review on a PR that isn't too large in, say, 1-2 days or less.
| If you have PRs sitting in review for weeks at a time on a
| regular basis[1], one of two things is happening: either
| engineers aren't properly prioritizing code review, or your PRs
| are too large and/or have too many dependencies.
|
| Which brings me to the second thing you can do, which is make
| PRs that are as small and simple as possible, but no more.
| Minimize dependencies to help make your PRs understandable.
| And, it should be fairly obvious, but the fewer lines of code
| (including config changes) you have in your PR, the easier it
| will be to review, so, the faster it will get done.
|
| Code review is also a great opportunity to learn and teach.
| Every senior+ engineer should be reviewing code frequently,
| IMO.
|
| Finally, yes, code reviews will slow you down if your basis for
| comparison is just "engineers merge code when they think it's
| ready." That's intentional, and good. The cost is more than
| paid for by better code quality, leading to fewer instances of
| "here's a PR to fix the issues with my previous PR."
|
| ---
|
| [0]: https://news.ycombinator.com/item?id=29801451
|
| [1]: I've only had this happen once or twice in my career so
| far. In each case, it was because the code _surrounding_ the
| code I was working on was not stable and kept changing. These
| were clearly exceptional instances.
| erpellan wrote:
| That final, 1 word sentence is one of the major problems of
| PRs. An engineer might spend several hours really thinking
| through a problem, talking with colleagues, whiteboarding
| options and coming up with a workable solution that addresses
| all the obvious issues and a bunch of non-obvious ones.
|
| Only for a drive-by take-down by someone with none of the
| context. It's a totally asymmetric investment of time. Even
| helpful review comments might only take a minute to write but a
| day to incorporate.
| kps wrote:
| > Only for a drive-by take-down by someone with none of the
| context.
|
| Then write it down. If the code reviewer can't follow what's
| going on, what hope is there for the new hire looking at it
| six months from now?
| ipaddr wrote:
| Because looking through 6 month old prs are what new hires
| are doing to learn the codebase?
| yxhuvud wrote:
| Sure. If they are good and have a decent grasp of using
| version control then will look at the history of a
| certain piece of code if they don't understand why it is
| the way it is. I do it often, even when I'm no longer a
| new hire.
| detaro wrote:
| Not necessarily looking through PRs, but presumably they
| need to work with actual code in your code base that was
| merged at some point, unless your product is growing so
| fast that new people just write new code all the time?
| (Same applies for not-new coworkers that now need to
| touch that code area anyways)
| fragmede wrote:
| Running through `git blame` and looking at the commit
| message, the PR, and the PR comments is a very
| _practical_ way to learn about a codebase! Beyond some
| high level code organization stuff that exists in a
| readme, learning a codebase is really about getting a
| history lesson of how the product evolved, the company
| pivoted, and the team re-org 'd.
|
| It'll be 6-months if you're lucky. Try figuring out why
| there's a particular "if" clause, one or two or four
| years later. Software maintenance, especially when the
| original author has moved onto another
| team/organization/company/career, is a frustrating art of
| almost remembered stories and hoping that you can figure
| out Chesterton's fence, lest it become Chesterton's
| barbed wire. The worst is when you fix a small bug with
| code and introduce an even bigger big in the process.
| lalaithion wrote:
| That is one of the things I do to learn new codebases.
| sanderjd wrote:
| Is this a common practice? I've never worked under a code
| review process where the accept / reject decision was being
| made by "drive-by" reviews from "someone with none of the
| context". It has always been team members with a lot of
| context reviewing each others' code.
| detaro wrote:
| > _Only for a drive-by take-down by someone with none of the
| context_
|
| If that's a regular problem that's a culture problem.
| Starting with "if you've been talking with colleagues about
| your problem, why is someone with no context reviewing the
| result?", people not investing time in reviews, people doing
| "take-downs" instead of asking questions if they don't
| understand things, hold up merge for non-urgent concerns ...
|
| > _Even helpful review comments might only take a minute to
| write but a day to incorporate._
|
| Either the feedback is worth the investment of the day of
| time, then no problem, otherwise it's not that important and
| doesn't need to be done (or depending on what it is can be
| done later or ...)
| dromtrund wrote:
| > drive-by take-down by someone with none of the context.
|
| If this is an issue in your workday, you should bring it up
| with management or the individual rejecter. If someone has
| the authority to kill PRs, they should also be required to
| put in the effort to understand it and/or be available for
| discussion at an earlier stage of the development process.
| PRs shouldn't ever be rejected without mutual agreement -
| neither internally in an organization, or in an open source
| project.
| physicsguy wrote:
| I require PRs with code review on projects I am an admin on
| because I don't trust myself. I sure as hell don't trust other
| people!
| AnimalMuppet wrote:
| It's worse than that. I don't trust _me_. You shouldn 't
| either. _I_ need the review.
| mumblemumble wrote:
| We (at least should) also do code reviews in order to make sure
| everyone understands the code.
|
| On my team, the majority of code review comments are not
| discussing potential bugs, they're making sure everybody knows
| how the new thing works, why it was designed that way,
| implementation tradeoffs, etc. All that discussion is extremely
| valuable over the long run. For example, it means that nobody
| has to pay attention to email while they're on vacation in case
| something goes pear-shaped with a module they implemented.
|
| That said, I 100% agree with the article's concluding paragraph
| about YMMV. Raycast is operating in an entirely different
| business domain from the one I'm in. It sounds like the reasons
| why Raycast likes their way of doing things don't really apply
| to us, and I don't think the reasons why we like our way of
| doing things don't really apply to Raycast.
| brunojppb wrote:
| Early in my career I was like "cool, I can just merge this to
| master". Now after working in several projects and in several
| small and large teams, I am like "Gez, I can't merge this
| straight to master. I need a second pair of eyes on this"
| jolux wrote:
| 1. Let's separate the idea of code review from the GitHub Pull
| Request feature. They are often coupled, but they don't need to
| be. You could easily have a policy and a workflow that requires
| reviews, but doesn't care whether they happen before or after a
| merge. I'm pretty sure this is how things work at Etsy, and I
| know there to be a diversity of code review practices more
| broadly in high-performing engineering organizations.
|
| 2. Of course code reviews don't catch _all_ bugs. This is the
| same argument that is frequently used to reject static type
| systems and memory safety. The purpose of code review is to
| ensure that _more_ bugs are caught, the kinds of bugs that can be
| caught through code review. It also helps ensure that a high
| level of code quality is maintained, and that at least two people
| understand how a change works, which is good for team cohesion
| and resilience.
| siva7 wrote:
| He basically described the classic subversion workflow using git.
| I feel like people rediscover subpar approaches using
| technologies which intended to evolve those approaches into a
| better workflow
| mixedCase wrote:
| I'm glad this works better for them, or at least they feel that
| it does. I'd also bet that they're either a small minority of
| teams that jell so well that they manage to scale a codebase
| without it turning into a disgustingly inconsistent mess; or that
| indeed their codebase qualifies as such or is still too small for
| the effects to be very visible.
|
| Code reviews are not a tool intended mainly to catch bugs. Types,
| automated tests and manual tests are going to do the bulk of your
| bug-catching after all. Code reviews' more relevant purpose is to
| act as QA for _the code itself_ far more than it is to verify if
| the code achieves what it sets out to do. This is in service of
| both maintainability (which impacts your ability to make future
| changes and staff retention) as well as performance (by
| introducing the chance for another dev to detect suboptimal
| approaches).
|
| Once your teams grow, code piles up, and static analysis can't
| cover all your bases, either your team corroborates the code that
| you write matches/defines group standards; or each developer's
| careful design accumulates as "organic growth" carefully duct
| taped together to barely work.
|
| And just to state the obvious, code reviews are merely _a_ tool
| to achieve this. Pair programming is another.
| commandlinefan wrote:
| > Code reviews are not a tool intended mainly to catch bugs
|
| Well, not to detract from your point (I agree), but what little
| research has been done in this area suggests that code reviews
| are actually one of the _best_ ways to catch bugs:
| https://kevin.burke.dev/kevin/the-best-ways-to-find-bugs-in-...
| whoomp12342 wrote:
| IMO code reviews are a tool for mentorship and keeping
| architecture in line
| ipaddr wrote:
| They can also be used for bullying, micromanaging and other
| social constructs
| xboxnolifes wrote:
| something, something, quote about all tools being able to
| be misused.
| geuis wrote:
| Absolutely not. This is a recipe for disaster and a terrible
| example to set for any upcoming engineers that haven't had much
| industry experience yet.
|
| Code reviews help me be more confident in my own work. Even if
| 90% of the time it's fine, there's always that 10% where a second
| pair of eyes catches a mistake or offers a suggestion that makes
| the code even better.
| watwut wrote:
| > there's always that 10% where a second pair of eyes catches a
| mistake or offers a suggestion that makes the code even better.
|
| Testing is pretty awesome process to find bugs.
| Clubber wrote:
| QA departments are expensive. Execs in their infinite wisdom
| opted to dump that task on developers who earn 3x as much as
| QA people. Synergies!
| marcosdumay wrote:
| > who earn 3x as much as QA people
|
| And never learned how to properly test software... Or earn
| 10x as much and consider it not a priority.
| mdaniel wrote:
| > offers a suggestion that makes the code even better
|
| plus, software engineering is a team sport, so if something
| _works_ but is _opaque_ , that's a fine thing to point out in a
| review, otherwise one runs the risk of being The "Foo
| Component" Owner(tm) and that's usually no fun and is for sure
| not healthy for any reasonably sized org
|
| I am also a monster fan of the "Apply Suggestion" button in
| GitLab, which allows me to meet the reviewer half-way by even
| doing the legwork for the suggested change. If they agree, push
| button and we're back on track. It's suboptimal for any process
| that requires/encourages signed commits, but damn handy
| otherwise
| klabb3 wrote:
| Meh. If 90% of the time it's a mindless ritual there is
| probably a better way to achieve the same goal. Code reviews
| are a huge time sink, especially for unimportant style- and
| naming nits.
|
| Code review as mentoring can be great, if it's a directed 1:1
| effort. Usually, it is not.
| LaserDiscMan wrote:
| As well as a time sink, code review can be detrimental if
| it's used as a KPI gaming mechanism or for passive aggressive
| one-upmanship.
| watwut wrote:
| Hours long discussion about whether to call it "thing" or
| "thingId". True story.
| marcinzm wrote:
| >Code reviews are a huge time sink, especially for
| unimportant style- and naming nits.
|
| This isn't a problem with code reviews, this is a problem
| with your team's processes that code review has highlighted.
| These problems will show up in others ways if not during code
| reviews since clearly there's strong disagreement on coding
| styles and conventions. So go and fix or implement the style
| guides, linters and so on.
|
| This is like saying that the way to fix fevers is to get rid
| of thermometers.
| tuyiown wrote:
| > So go and fix or implement the style guides
|
| How do you make this converge across dev without endless
| debate or resentment ?
|
| > linters and so on.
|
| What happens when a rule has to change ? update the entire
| codebase impacting everyone with conflicts ? tolerate
| divergence existing code ?
|
| Talking about thermometers, are you actually tracking
| fevers, or merely minor bad breath ?
| atomashpolskiy wrote:
| The problem is not with code reviews but with how adversarial
| they have become. Instead of making the best attempt to
| comprehend and embrace the author's style and intention and
| finding compelling arguments for the question "why this code is
| probably fine and should be merged as-is?" people nit on all
| kinds of stuff, mostly highly subjective. Along the way they
| don't actually catch that many problems or bugs but instead new
| problems and bugs are introduced while implementing their nits. I
| agree with other posters in this thread that most people can't
| code review for crap and have very perverted idea about code
| review aims and goals.
| curious_cat_163 wrote:
| Code reviews help. However, it is unclear to me if PRs are the
| best way to do them. I often find a walkthrough on a screen share
| a lot more helpful to receive/give feedback. It also ends up
| saving time. The downside of this approach is that it has to be
| synchronous.
| renewiltord wrote:
| Most small tech companies probably operate in this manner. We do,
| too. The practice is that devs _do_ want a second pair of eyes on
| the code since they want other people to detect flaws in their
| assumptions or choices before there is pain.
|
| If I'm doing something trivial, I might just push to `master`.
| For instance, I don't run tests on a `README.md` change.
|
| Interesting to encode it explicitly, though. Strong culture
| choice, for sure. And probably well suited to their product since
| they are probably all dogfooding `master` and pushing only
| periodic tags off it.
| cdaringe wrote:
| > Pull requests don't prevent bugs Pull requests find bugs on
| every software and hardware team I have ever participated in. I
| cannot imagine living without this process. PRs increase
| knowledge and awareness of features. I fully concede that it is
| discouraging in some circumstances, but those tend to be the
| exception, not the rule. Most often it's discouraging around a
| core set of individuals who bring more dogmatism and policing to
| PRs, which aren't entirely welcome. Power to this team, but
| either they've discovered the holy grail, or they will be
| compensating for this in some other internal process, certainly
| not user feedback.
| geoah wrote:
| Couldn't agree more. I'm fairly certain that I'm a better coder
| and team member than I used to be because I was part of both
| ends of many many PRs.
| Someone1234 wrote:
| When my code has been code reviewed I've had bugs found,
| improvements made, and edge cases brought up. I genuinely believe
| my code is better for having been reviewed, and I work harder on
| the quality of my code because it is going to get reviewed.
|
| Now, code reviews aren't a panacea, there's an awful lot of bike-
| shedding going on (in particular whenever interfaces are
| reviewed) but they're a net positive in my view. It isn't about
| trust, it is about creating a quality product, along with
| postmortems (an under-utilized philosophy) and automated tooling
| (inc. testing, validators, etc).
| that_guy_iain wrote:
| > Pull requests don't prevent bugs.
|
| I remember once I was giving a talk on code review and I said
| that code review doesn't prevent bugs and people were shaking
| their heads. Everyone hangs on to this one thing, but if you look
| at the majority of pull requests that have been approved you'll
| see a lack of comments about pontential bugs. Also, if you do see
| a comment about bugs they'll often be disregarded. This drives me
| up the wall when I've previously pointed out bugs and they've
| been found to be actual bugs and had to be fixed.
|
| I think the biggest problem with code reviews is it often becomes
| adversarial and people want to come out on top. I've had it more
| than once that someone has suggested another way of doing
| something, when I asked what the benefit of that way was or what
| was the downside of the way I was doing it no answer was forth
| coming however I was expected to implement their way. This
| becomes enraging when you point out flaws in their way but they
| can't find any benefit of their way.
|
| As well as, often people just don't want to do the extra work.
| You point about a bunch of small improvements. It's a pain.
|
| You end up with people asking for code style changes even tho
| there is a code style and the changes they're asking for aren't
| in those guidelines-
|
| In my opinion, code review is a massive waste of time that is
| often done purely as cargo cult. People know it's a good thing to
| do but people have no idea how to code review and what is and
| what is not benefical during code review.
| baskethead wrote:
| It sounds like your company has a shitty culture. I've never
| had issues with code reviews in the companies I worked at. One
| company was particularly rigorous, where we had to print it ou
| on paper, and a room of engineers would go over things line by
| line. But that also had the best engineered software I had seen
| in my career.
|
| Any issues of style should be in the style guide. Anything not
| in the style guide can be ignored. Or the style guide should be
| updated.
|
| There should be a distinction between opinion and technical
| problems. If you suggest a change that is opinion-based, then
| it can be taken or ignored. I never comment on suggestions,
| because it's a waste of my time and theirs. If you are
| suggesting a chance that is a technical problem, then that
| should be changed. If the person doesn't want to make the
| change for the technical problem, then that's a culture problem
| in the team or company.
| curious_cat_163 wrote:
| > I think the biggest problem with code reviews is it often
| becomes adversarial and people want to come out on top.
|
| Is that really a problem with code reviews? Or, is that a
| cultural issue for a given team?
| zzzeek wrote:
| > I remember once I was giving a talk on code review and I said
| that code review doesn't prevent bugs and people were shaking
| their heads. Everyone hangs on to this one thing, but if you
| look at the majority of pull requests that have been approved
| you'll see a lack of comments about pontential bugs.
|
| that's because the UX for github PRs sucks. Use Gerrit instead.
| It's not as pretty but it's super productive.
|
| > I think the biggest problem with code reviews is it often
| becomes adversarial and people want to come out on top. I've
| had it more than once that someone has suggested another way of
| doing something, when I asked what the benefit of that way was
| or what was the downside of the way I was doing it no answer
| was forth coming however I was expected to implement their way.
| This becomes enraging when you point out flaws in their way but
| they can't find any benefit of their way.
|
| that's not a problem caused by code review, it's caused by
| people who don't know how to work with others (or otherwise
| simply refuse to). to the degree that the code review process
| shines a light on this, that's good news, because that's a
| people problem that should be fixed.
|
| > As well as, often people just don't want to do the extra
| work. You point about a bunch of small improvements. It's a
| pain.
|
| when you look at a code review and you see problems in what's
| there, catching them in the code review is great, because that
| means less chance of having to fix it later.
|
| if you see a code review and nothing's wrong with it, you just
| approve it. sometimes you can note things as "nits", meaning,
| fine for now but maybe next time do it this way.
|
| > You end up with people asking for code style changes even tho
| there is a code style and the changes they're asking for aren't
| in those guidelines-
|
| syntactical style and formatting should be automated, if not
| with tooling that actually rearranges the code in that way
| (e.g. [black](https://github.com/psf/black) for Python) then at
| least including well tuned linter rules. We use black, my own
| zimports tool for sorting imports, flake8 with four or five
| plugins as well as "black check", so we spend exactly zero time
| dealing with anything to do with style or code formatting,
| including having to even type any of it. if people are fighting
| over code style then there need to be tools that lock all of
| that down so there's no disagreements.
|
| > People know it's a good thing to do but people have no idea
| how to code review and what is and what is not benefical during
| code review.
|
| I work on the Openstack project and that is where I learned to
| do code review with Gerrit, it is completely essential for
| large projects and is an enormous productivity enhancement.
| Decades ago before CVS was even invented, the notion of using
| source control seemed crazy to me. That position was obviously
| insane, even though we were stuck using Visual Source Safe at
| the time. That's how it feels to me to be doing a real project
| today without code review and formatting tooling in place. Like
| even for myself, if working alone, for a large project with
| lots of changes I still use a code review tool, just to review
| _myself_.
| that_guy_iain wrote:
| > that's because the UX for github PRs sucks. Use Gerrit
| instead. It's not as pretty but it's super productive.
|
| That won't affect the quality of what people find and comment
| about.
|
| > that's not a problem caused by code review, it's caused by
| people who don't know how to work with others (or otherwise
| simply refuse to). to the degree that the code review process
| shines a light on this, that's good news, because that's a
| people problem that should be fixed.
|
| You say that like fixing people is an easy thing to do. It's
| the hardest thing to do. And the fact I've seen this at so
| many companies/teams leads me to believe that it's an
| extremely common thing.
|
| > when you look at a code review and you see problems in
| what's there, catching them in the code review is great,
| because that means less chance of having to fix it later. >
| if you see a code review and nothing's wrong with it, you
| just approve it. sometimes you can note things as "nits",
| meaning, fine for now but maybe next time do it this way.
|
| Yea... Those aren't getting done at 9/10 companies. They're
| getting ignored.
|
| > syntactical style and formatting should be automated, if
| not with tooling that actually rearranges the code in that
| way (e.g. [black](https://github.com/psf/black) for Python)
| then at least including well tuned linter rules. We use
| black, my own zimports tool for sorting imports, flake8 with
| four or five plugins as well as "black check", so we spend
| exactly zero time dealing with anything to do with style or
| code formatting, including having to even type any of it. if
| people are fighting over code style then there need to be
| tools that lock all of that down so there's no disagreements.
|
| Yea, we have those, I still get asked for stupid ass code
| style changes at every company for the last 8-years. Why?
| Because people can spot them and understand them. But can't
| code review for shit. They can't understand the idioms for
| the programming language, they can't understand how certain
| paradims work, they don't pay attention to root causes of
| bugs, they don't know how to design a database, they don't
| pay attention to performance, etc. I would say 80% of the
| industry can't code review for crap. And that is the biggest
| problem and because they can't do that all the other problems
| are just symptons.
|
| > I work on the Openstack project and that is where I learned
| to do code review with Gerrit, it is completely essential for
| large projects and is an enormous productivity enhancement.
| Decades ago before CVS was even invented, the notion of using
| source control seemed crazy to me. That position was
| obviously insane, even though we were stuck using Visual
| Source Safe at the time. That's how it feels to me to be
| doing a real project today without code review and formatting
| tooling in place. Like even for myself, if working alone, for
| a large project with lots of changes I still use a code
| review tool, just to review myself.
|
| This doesn't dispute or refute my point. It merely says in
| some cases it's used well. It doesn't remove the fact that
| majority of the time it's cargo cult and the majority of
| people who code review can't code review for crap or the fact
| that a high percentage of people just skim code review.
| azov wrote:
| Coming up next: "We don't use source control. Git just slows us
| down. Our engineers don't want to spend time writing commit
| messages when they can be writing features instead. We keep all
| our code in a shared folder and we _trust_ our team members to
| not mess it up!"
|
| Well, this can be done. Lots of software was written before code
| reviews or source control existed. But I'm not longing for that
| time.
| codeptualize wrote:
| Good CR's are great, bad CR's are an incredible waste of time and
| you might as well not do them at all.
|
| We had a similar system where you trust people to know when you
| can skip the CR, worked great. We also ranked CR comments on
| importance, focussed on preventing issues, and avoid subjective
| discussions as much as possible. It meant CR's were mostly about
| bug catching, sometimes constructive conversations weighing pro's
| and cons, and other times making suggestions that may or may not
| be applied.
|
| It's so much nicer than a dogmatic nitpicking CR culture that
| completely forgets about what's valuable and not. Maybe it's my
| relative small sample size but I feel they always go together.
| windows2020 wrote:
| I've seen a number of flavors of code reviews in practice:
| * Superficial - Omission of optional lagging comma * Valid
| in context - Quadratic logic on small finite set * Ensure
| intention - Task not completed/completed with adverse effects
|
| Each takes progressively more effort, skill and context
| knowledge. There's a constant battle between resiliency and
| efficiency. There also tends to be a gradient of skill involved.
| Context is king when it comes to an opinion on this.
| simonbarker87 wrote:
| I can see their point of view but it sounds like they focus on
| the downsides of code review and not the benefits.
|
| Code reviews increases awareness of changes, helps with
| desiloing, gives junior devs a chance to see how others write
| code in their own time, reduces chance of mistakes, encourages
| collaboration.
|
| To stop the endless ping ponging back and forth I advocate for
| Must/Should/Could in all code reviews. So long as everyone gets
| it, it massively speeds up code review and increases team
| happiness around the process.
|
| https://careerswitchtocoding.com/blog/moscow-the-best-code-r...
| 0xbadcafebee wrote:
| > we trigger an internal release every night which includes all
| changes that got committed during the day [...] This allows us to
| test new changes within 24 hours. [...] This workflow helps us to
| consistently ship updates every other week.
|
| Every merge to _main_ should ship immediately. If it doesn 't,
| you still don't have enough trust. Waiting 24 hours to test is
| just arbitrarily slowing you down. If you test 3x a day, you
| debug 3x faster, meaning you ship 3x faster. Test immediately,
| revert immediately, re-run tests on every merge.
| larsrc wrote:
| Apart from the other reasons for code reviews already mentioned,
| there's this: an inexperienced developer (in a particular area)
| may not _know_ when a code review is appropriate. A change that
| looks innocent may have wider consequences than expected. Code
| reviews spread knowledge, and knowledge is the true output of
| software engineering.
| [deleted]
| pxllh wrote:
| I always really enjoy reading both the Raycast and Linear
| engineering blogs because they've really got a great approach to
| empowering distributed teams, and giving engineers the autonomy
| to work well.
|
| I can't necessarily say this approach would work everywhere but
| it's nice to see companies critiquing and challenging
| methodologies.
| tombert wrote:
| I think I largely agree with this article.
|
| When I worked for Apple, we had a strict "all code needs to be
| reviewed" policy, which I had no problem with. Then, after being
| there for about 1.5 years, I make a PR, ask two different people
| to approve it, which they do, and it gets merged. A week later,
| the code is released, it looks like my code caused a fairly major
| bug.
|
| My manager's manager and my manager had a meeting with me, and
| told me that this was unacceptable. I ask "what exactly was I
| supposed to do differently? Clearly this bug was sneaky enough to
| where two other people also didn't see it." Their response was
| "the code needs to work, you wrote the code, the code was sloppy.
| This is your fault." Apparently they took the bug pretty
| seriously, since it was actually mentioned in my yearly review.
|
| This is fine, I probably should have tested the code a bit
| better, but it always annoyed me that my "sloppy" code managed to
| pass code review [1], and yet I'm the only one to get in trouble
| over it. If the PRs aren't catching bugs and I still am going to
| get in trouble, then I'm failing to see why I should bother
| waiting an hour for someone to look through it. Instead, why not
| make it so we can merge quickly, test it quickly, and revert it
| if something breaks?
|
| [1] Obviously I'm biased here, but I honestly think that in this
| particular case I was pretty easily the least-liked person on the
| team, and it was easier to throw me under the bus than anything
| else.
| igetspam wrote:
| You had shitty management.
| tombert wrote:
| No argument here. I should probably have switched teams,
| since some of my friends who are still at Apple seem to have
| had a better experience.
| dsjoerg wrote:
| > test it quickly
|
| Yes, in your scenario the lack of testing stands out as the
| major opportunity for improvement. If a bug is so important
| that it's mentioned in a performance review, then it's
| important enough that there should be tests that would have
| caught it. Automated preferably, or manual if necessary.
|
| And everyone involved with the software, from you on up and
| sideways, should be calling for this testing. It's clearly
| important / worthwhile!
| baskethead wrote:
| I don't know what your point is. You would have gotten in
| trouble whether or not code reviews were present. If you're
| saying that code reviews should not be default, then the entire
| blame would rest on your shoulders anyway. Are you saying you
| wish you got in less trouble at Apple and the code reviewers
| should have gotten in more trouble for missing YOUR bug?
| tombert wrote:
| > Are you saying you wish you got in less trouble at Apple
| and the code reviewers should have gotten in more trouble for
| missing YOUR bug?
|
| If "my" bug was a result of sloppy code, my PR should have
| been rejected.
|
| I don't really think anyone should get in trouble for a code
| bug (at least in a majority of cases), but yes, if this bug
| was obvious enough for me to be in trouble for not catching
| it, then the people approving the PR should share
| responsibility for it.
| pearjuice wrote:
| Code reviews make sure the other party at least thinks a bit
| about the code they write. "Someone is going to look at this,
| let's at least tidy it up or not be cobbled together
| underperforming spaghetti". Sure, you can trust your co-workers
| but do you trust them enough that they have that thought every
| day of the year?
| foxbarrington wrote:
| Agreed. It's amazing how code magically gets better when you
| know someone else will be reading it.
| jsiaajdsdaa wrote:
| My current company (dinosaur insurance company) has a one
| developer per service mapping right now and it is absolute HEAVEN
| on Earth.
|
| The best services rise to the top, and the other ones are
| refactored until they are ready to be consumed.
| system16 wrote:
| I guess it depends a lot on the company's dev culture or team
| dynamic.
|
| In my current company, our code reviews are essentially a second
| set of eyes by a peer. When things are "flagged" in code review
| it's never confrontational. Mainly they provide insight for those
| who haven't worked on the code base as long as to "why" things
| are the way they are, and it can open up discussions for
| improvements (or at least things to ponder for the backlog.) The
| odd time neat language or platform tricks are also learned.
|
| I've worked in other places though where code reviews are done by
| rockstars or ninjas just looking for a reason to criticize or
| find fault with another's code so they can stroke their own ego.
| And if their code is ever questioned, expect a tantrum. Toxic
| environments like these say more about the company and people
| there than code reviews though.
|
| Looking forward to the follow-up "no QA by default".
| notmyfuture wrote:
| For a small team of capable engineers, working on something that
| has low consequence for any single change failing plus a strong
| business motivation to move fast: sure, do without code reviews
| if you want.
|
| There is no universal "best practise" for building software,
| there is just whatever works best for your context. Practices
| can, and should change as your business context does. There are
| some things that are net beneficial for a team in the majority of
| situations though, I'd consider code reviews one of these things
| and make that the default.
| alexhf wrote:
| I wish this talked about the engineering time lost when a bad PR
| makes it to production. Fixing those may eliminate any time gains
| from skipping code review. Also, this violates security
| compliance standards (e.g. SOC2).
| arnvald wrote:
| Previous discussion (from June 2021):
| https://news.ycombinator.com/item?id=27606066
| anthonyskipper wrote:
| Luckily Raycast work in an unregulated industry. In a lot of
| regulated industries this would not be an option as 4-eye reviews
| are a mandatory policy requirement.
|
| I also don't know how smart it is to brag about, seems like just
| asking for something to go wrong.
| bob1029 wrote:
| There is a Russian proverb I heard used very early on in my
| career that has really stuck with me over the years:
|
| "Trust, but verify"
|
| I think a lot of other commenters nailed the trust equation in
| more verbose terms. I would add that in some industries or for
| certain customers, a 2+ person code review rule is _mandatory_
| for compliance, regardless of any perceptions around how mundane
| the changeset might be.
| dan-robertson wrote:
| I'm curious how they avoid pushing changes that break the build.
| Maybe they have some other reason that this doesn't happen or is
| hard but having a large number of people trying to collaborate on
| a constantly broken repo is not a recipe for productivity.
|
| I also wonder if there are typically 0 reviews or typically 1 --
| that is, are people even reading their own changes before they
| push them.
| ReleaseCandidat wrote:
| > I'm curious how they avoid pushing changes that break the
| build.
|
| By not pushing changes that break the build. In other words:
| that does not work if you can't test (actually test, not just
| try to build) your code before pushing it to main.
| AlfeG wrote:
| Is there anyone on the planet, that always run all tests
| locally even for last second minor change that definitely
| will not break a build?
|
| Especially funny for distributed around globe teams. When You
| spent half a day fixing a build issue from the "night"
| workers. Have eat a lot of this.
| yupper32 wrote:
| > Pull requests don't prevent bugs.
|
| Is this some anti-vax satire?
|
| Of course pull requests (code reviews) reduce bugs. And of course
| some slip through. It doesn't need to be 100% to be useful.
| canyonero wrote:
| Yeah -- I LOLed at this one because I've caught at least
| hundreds, maybe thousands of bugs during review on pull
| requests.
|
| It's weird and silly to expect prevention with tool that is not
| designed to prevent. Peer review (and the PR model) exists as
| quality control tool for many different aspects of someone
| else's work.
| Abroszka wrote:
| I haven't seen any research that supports either. I wouldn't be
| surprised to learn that code review does nothing other than
| share knowledge, or to learn that it reduces bugs by 50%. I
| just honestly have no idea, I do code reviews, because we do
| code reviews.
|
| In fact I have seen alarmingly little research about code
| management. Code review, standup, agile, etc. does any of it do
| anything useful? I only came across anecdotal evidence which
| can be dismissed.
| buttersbrian wrote:
| I think pull requests are synonymous with code-reviews here.
|
| I think it is unquestionable that code-reviews catch bugs.
| Its value as a use of time is another question.
| Abroszka wrote:
| > I think it is unquestionable that code-reviews catch
| bugs.
|
| Yes, it does catch some, but does it catch more than no
| code review? There is no point in catching bugs if it also
| creates bugs to catch.
| yupper32 wrote:
| Sorry, are you saying that it's possible that code
| reviews cause more bugs than they solve?
|
| That's absurd...
| Abroszka wrote:
| Why would it be absurd? If there is a process to reduce
| risks, then people take more risks. I have been guilty of
| submitting code review when I'm not 100% sure it's
| perfect, just because I know that there is a code review
| process. So this definitely exists, not sure how common
| it is though.
| deckard1 wrote:
| Also, the trick to code reviews is to leave in a few low-
| hanging obvious bike sheds. The reviewers will tell you
| what color to paint them. Done.
|
| I'm being slightly sarcastic. But not sarcastic to the
| point that I haven't done just that. Just as the nail
| that sticks out gets hammered, the code that is _too_
| perfect gets increased scrutiny.
| Verdex wrote:
| Perhaps unlikely. Or really surprising. But not absurd.
|
| Imagine some reviewer who rejects all code reviews that
| don't have some recognizable pattern in them from the
| gang of four book. Fully complete and working code gets
| hacked up last minute to accommodate this person who has
| more seniority than sense.
|
| Or ... maybe someone who always want there to be a single
| return in every function. Or heck, someone who demands
| that you always do early return. Either way they demand
| that some carefully crafted code is swapped over to the
| equivalent dual. And during that transformation a mistake
| is make that nobody notices.
|
| I think the point is that the science way to indicate
| that code reviews work is to actually do the experiment.
| Instead of just saying, "why that's absurd that a code
| review could cause more defects."
|
| I mean isn't that how hand washing got introduced into
| medicine? "Hey everyone, let's try washing our hands!"
| "Why, the hands of a gentleman are always clean. It's
| absurd to suggest that we should wash our hands."
| buttersbrian wrote:
| If it catches ANY bugs it is objectively better than no
| PR/code-review from a bug standpoint. Unless you're
| arguing that pr/code-review creates more bugs than it
| solves?
|
| Again, I think the question is value. Is a dev's time
| best used in code-review vs. something else? It's almost
| certainly the case that time is better spent elsewhere
| depending on the develop and needs or the organization.
| munchbunny wrote:
| I think the question is the same one as "do bike helmets
| reduce bike injuries?" I.e. without code reviews, is
| everyone more careful? I can't think of any proper
| academic research that answers the question.
|
| In my anecdotal experience, there is at least one class
| of bugs that code reviews are good at catching that a
| large expenditure of self-review effort often doesn't
| catch: security bugs.
| Abroszka wrote:
| > If it catches ANY bugs it is objectively better than no
| PR/code-review from a bug standpoint.
|
| No, that's false. That's only true if you also assume
| that people write the same quality code whether there is
| a code review process or not. Which might or might not be
| true, no idea.
| cunac wrote:
| if anything I would think that person would write same
| code but without PR would lost the opportunity of having
| better quality. People are blind to own mistakes. I can't
| count how many times I would stare at a code not seeing
| something and other person would spot problem almost
| immediately.
| dovetailed wrote:
| przemelek wrote:
| I used to love code reviews when I worked for Motorola in 2004,
| it was doing wonders, we observed showing up "phantom developer"
| where mix of inputs from different people was causing new look on
| code. Those were Fagan-like code inspections. A lot of paperwork
| (we were doing formal reviews on paper, with one person in role
| of reader), but it was working. This whole paperwork was forcing
| people to follow rules and it seemed to work (but was sometimes
| very painful). Latter in other companies I found out that code
| reviews are in most cases bullshit placeholders, and in many
| cases are only for making some developers feeling more important,
| or other devs not loosing contact with code. I saw most stupid
| things in code moved to position of examples how to write code
| only because of code reviews... Good code reviews are doing
| wonders, but as I can see without very strong culture in dev
| teams and whole company those code reviews don't add anything
| useful. And this strong culture is difficult. From observations
| it seems that this is easier to build in team culture to work
| with "master/main only" without branches and code reviews, than
| to build culture of code reviews which are working. From my
| observations most of comments in code reviews are variations on
| "I would code it in different way", or "why you are doing it in
| this way/I don't like your variable name" of course usually
| written in less direct way. Last year problems with Log4J2 showed
| that even in OpenSource peer reviews don't help. Good developers
| let to introduce to one of most used libraries something what
| never should be introduced there. For sure variable names were
| nice, but somehow whole "why we are adding this" was lost,
| because devs were looking only for some easy to spot things.
| So.... yep, good code reviews are important, but my guess is that
| most of code reviews are only to make some devs happier that they
| still know what happens in code, and that variables are named in
| acceptable by them ways....
| micromacrofoot wrote:
| Good luck getting any sort of security certification without
| reviews. This works fine for small companies, but once you start
| getting customers at a certain scale it will be a dealbreaker.
| Run without reviews as long as you can, it feels great!
| zzzeek wrote:
| so they didn't like github PRs so...they ditched code review
| altogether? how strange and sad. there's lots of tools that can
| be used for code reviews that don't have any bearing on whether
| or not you also use github for source control.
|
| We at the SQLAlchemy org actually have Gerrit integrated with
| Github PRs so you can use both UXes for the same patch at once,
| and other orgs do this too.
| canyonero wrote:
| I could see this working well for teams that are engineering led
| and where customers don't expect a ton from the software. If
| you're trying to ship fast and get to market quickly, then fine,
| skip the tests, skip the code review.
|
| Once you have lots of engineers shipping many different apps and
| need to work within and across teams. This system is not going to
| be fun. When a nasty bug ships and you're responsible, you'll
| wonder "should I have requested review for that one?" When one of
| your colleagues ships a few bugs that force you to paged during
| your on-call. This system is likely to erode trust on both ends.
| IMO, a no code-reviews model is going to stunt junior engineers
| career growth as they will not be performing what is common
| practice on software engineering teams. It's also going to keep
| out many other stakeholders who may wanna weigh in on the
| software you're delivering, be it UX, Accessibility, Security,
| Documentation, Product experts. Pull requests keep others on your
| team in the loop and educated.
|
| I agree that this model gives you the advantage of speed, but I
| don't think it builds trust. I trust my colleagues and we do pull
| requests. I don't feel like I'm missing trust because I can't
| push to `main`. Code reviews have very little to do with trust.
| They serve as a communication tool and serve a tool to give the
| best possible experience for customers and provide an opportunity
| for alignment with our colleagues.
| sbeckeriv wrote:
| The only thing I agree with from this "Make your own rules". I
| feel like it is click bait otherwise. Reading this I would apply
| for a job here personally.
___________________________________________________________________
(page generated 2022-01-04 23:00 UTC)