6 ms·
Code reviews: A success story
- moshegramovsky 2y agoAI is pretty good at code reviews. For reference I use chatgpt and Gemini. It's very helpful. An AI tool that could convert large scale changes into a set of small commits would be amazing.
- nogridbag 2y agoAre you using AI for public contributions? Or a private repo? How does it deal with things like project conventions? e.g. "Currency should be represented using a Money class", "This method should use our utility class", etc. Do you upload the entire project's source code?
- moshegramovsky 2y agoThe review you get depends what questions you ask. For example: I wrote a class that wrapped a std::vector<T> as a private data member and it pointed out that it would be nice if I implemented support for accessing the iterators and the array subscripts. It made these remarks based on how I was using the object. I have uploaded an entire repo to Gemini (as a single file) and asked for broad and fine reviews. It's really quite good.
- nogridbag 2y agoYeah I've heard of uploading entire repos several times now and I'm really surprised there isn't more concern about this. Prior to this, the thought of any developer uploading source code to any 3rd party would likely result in a bad outcome for that employee. And now we have juniors through seniors uploading not only single classes but entire projects. I suppose it's one thing if the company has some sort of corporate account with an AI partner and you login with your company credentials. But I doubt this is the case for most devs.
- jasonpeacock 2y agoWhile I am a proponent of code reviews, what this article actually described is mentoring of a junior engineer by a senior engineer, and requiring effective testing to be implemented alongside the feature. It also shows a broken culture where the other reviewers were not engaged and committed to the success of the feature. When a bug escapes, it's both the author _and_ reviewer at fault.
- hnlurker22 2y agoHe's obviously self-praising for a promotion or looking for another job. I don't think it should be taken seriously on the matter of the effectiveness of code-reviews
- jmmv 2y agoNone of the two were at play when I wrote this. Try again.
- deleted 2y ago[deleted]
- kyt 2y agoDisagree. It's only the author's fault. We can't expect engineers to write code and find every last bug in other people's code especially when PRs can be thousands of lines to review. A broken culture would be expecting engineers to find every last bug in other people's code and blaming them for bugs.
- Arainach 2y agoWhen issues happen, it is almost never just one person's fault. Just trace the Whys: 1) Why did the bug happen? These lines of code 2) Why wasn't that caught by tests? The test was broken / didn't exist 3) How was the code submitted without a test? The reviewer didn't call it out. Arguing about percentages doesn't matter. What matters is fixing the team process so this doesn't happen again. This means formally documented standards for test coverage and education and building team culture so people understand why it's important. It's not "just the author's fault". That kind of blame game is toxic to teams.
- strongpigeon 2y agoOnly somewhat related, but I'd pay decent money to have access to the whole Piper/CitC/Critique/Code Search stack. As much as I've tried to like it, I just don't really like Github's code review tool.
- tristanb 2y agoShameless plug, but we built http://CodePeer.com http://CodePeer.com - to bring Critique like features to everyone else. Take it for a spin if you like!
- mtlynch 2y agoFormer Googler. I also miss Critique/Gerrit. I've tried a bunch of alternatives, and I like CodeApprove: https://codeapprove.com/ https://codeapprove.com/ It's great if you have a team that does code reviews. It works less well for reviewing contributions from external contributors on an open-source project,a as the contributor likely just wants to get their PR merged and doesn't want to learn a special reviewing tool. No affiliation, just a happy customer.
- spankalee 2y agoI stopped reading after that opening paragraph. I don't know of anyone I take seriously who thinks that code reviews are bad practice or pure red tape.
- hnlurker22 2y agoThankfully I'm not working with you
- hamandcheese 2y agoMandatory code review definitely creates red tape. Every place I've been with mandatory code review, I always see people "looking for a stamp". At my current job, code review requirements are set on a per-folder basis, with many folders not requiring a review. People ask for a review because they want a review (or, sometimes, they dont. For example, I don't ask someone to review every quick one-liner of the systems I am an expert in).
- yuliyp 2y agoSure there's some subset of commits where all that makes sense is a stamp. On good teams it at least still ensures that two people agree that a stamp is appropriate. Knowing that you need to be able to convince someone it's good before it goes in is a good forcing function reducing sloppiness and laziness.
- hamandcheese 2y ago> On good teams it at least still ensures that two people agree that a stamp is appropriate. That is a good point!
- rapfaria 2y agoSame. > Code reviews have a bad rap: they are antagonistic in nature and, sometimes, pure red tape. I wonder if folks know that this is a job? What are you gonna do, not do it? Cry at night because you forgot for the hundreth time to add token strings to translation files and not be hard-coded? Come on.
- hnlurker22 2y agoThe author describes how his code reviews that he gave others are successful from his own point of view.
- awkward 2y agoDon't worry, he also asked his own reviewee, who said the reviews were helpful and in no way obnoxious.
- hnlurker22 2y agoMy statement still stands true regardless. Not worried.
- jmmv 2y agoI did not ask. This is feedback that the reviewee gave me years after we both left Google.
- pavel_lishin 2y agoBut he does back it up with actual facts (as far as we can trust the author to tell the truth) - the feature that the author gave feedback on shipped without any issues. (The article actually doesn't say whether A was fixed-and-bug-free before B shipped, but it certainly sounds like B was less stressful to ship.)
- IMTDb 2y agoIt’s also a biased view. The author admit that the feature he was involved in took longer to ship initially. Depending on the environment this can be an anti-pattern; don’t we say “release early, release often”. In the same vein; the author says that the other feature took several releases to be stable. Were the other release purely bug fixes or did that give the engineer a chance to get early feedback and incorporate that into the feature ? It’s clear that the author prefers a slow and careful approach, and he judges “success” and “failure” by that metric. It sometimes is the right approach. It sometimes isn’t.
- 2y ago
- frankfrank13 2y ago> I also pushed for breaking large changes into smaller commits, because it is impossible to do a thorough review otherwise. I've found this to be key for both giving and receiving feedback. Even small breaking commits are better in a lot of cases because they invite feedback in a way that 1000+ lines don't.
- dietr1ch 2y agoBreaking down the size of the change is truly important, otherwise it's easy to miss things and to also disregard them as little details when wanting to avoid blocking the whole change on a "small" thing (which may only seem small because the PR is now huge)
- findjashua 2y agoI miss Phabricator from my time at Meta so much, I made this to achieve a Phabricator-like stacked-commit experience via the git cli: https://pypi.org/project/stacksmith/ https://pypi.org/project/stacksmith/
- sqwxl 2y agoThis looks really interesting. I've been using a similar tool called spr https://github.com/spacedentist/spr https://github.com/spacedentist/spr for the last six or so months at work. I really like the stacked diff/PR workflow. But spr has a lot of rough edges and im on the lookout for a better alternative. Do you happen to know how your tool compares to spr? How production-ready is it?
- findjashua 2y agoI had checked out SPR, but found the workflow based around reordering history via interactive rebase unintuitive & clunky. That was the motivation behind building this on my own. I have been using this for a few months now, and it has served me well! I haven't spent much (any) time marketing it, so haven't really had any feedback from other users yet. Feel free to check it out & lmk if you have any suggestions. It's also open source, so feed free to open issue/PR on Github
- chw9e 2y agoIn my experience, mandatory code reviews seldom work very well. Usually it's either stamping PRs or you always run the risk of someone just blocking stuff in an unreasonable manner. For the most part, code reviews should be optional - if you want to get a review from someone, tag them on your PR and ask. If someone you didn't tag spots something and your PR landed, you can always figure it out and still make a fix. I will give an exception to maybe super fragile parts of the code but ideally you can refactor/build tests/do something else that doesn't require blocking code review to land changes.
- webdood90 2y agoI would like to offer a review on this article. Naming people and projects single characters makes it difficult to follow the content.
- tantalor 2y agoThey are not antagonistic in nature! Where did they get this idea?
- henshao 2y agoI have trouble with code reviews because I can't run the code from the review GUI, so I usually look at the tests run in CI or pull and run it myself, if possible. Is this not a problem other people have? Is this a tooling problem? By putting breakpoints in the code, and seeing what the changed lines do, I can compare the output line by line with my mental model of what should be happening, and this thoroughly helps me verify the changes.
- hakunin 2y agoI don't think I ever try to verify the correctness of the code in code review. That seems hopeless to me. The original developer should be assumed to produce working code, which is covered with tests and maybe will go through QA (if you have that). Of course there are occasional bugs you can spot, but the main goal is to review the architectural approach and code clarity. You can also note if something isn't properly covered with tests. If you managed to make any security, performance, or edge case handing suggestions, that's a nice bonus. Maybe we're in a very different context, but to me if you can't understand what the code does without a debugger, then you ask the author to make the code clearer.
- gct 2y agoMandatory CR is mostly a way for mediocre programmers to feel like they're contributing IMHO.
- zephyrkeane66 2y ago[dead]