I'm all for this if the person reviewing my code will know the context, history and all the details and conversations we had as a team.
But in order for that to work, I'd probably be taking most of this reviewer's time. And obviously in order for them to get up to speed with our practices, conventions, architecture, code style and whatnot, they'd probably need to start by doing a whole lot of development on our project first. At least several months. They could probably not do anyone elses code review then. So we'd have to pay them... one full time salary to be this coder-and-reviewer type person. I wonder what this service should be called.
I think I might be onto something here. "Full time developers as a service"
Having done more than a few PR reviews and code security reviews for their platform as an Android/Kotlin dev, I've found that the opposite problem is more common. A lot of organizations suffer from insular thinking and their own team often comments LGTM even if there's something glaring.
Writing reviews as an outsider, there's something freeing about knowing that you can review honestly and professionally and not overly worry that a colleague might get offended when you're simply trying to help. It's also not a chore anymore. Since the review is the job itself, it doesn't feel like a distraction. And since you're an outsider, you might know about best practices at your organization that the client hasn't been exposed to.
When I review code, I read the summary explaining what that org likes in a review, but I also make sure to include tools and practices that they might not be aware of. In many cases, I can see they're lacking automated static analysis like ktlint/detekt and point it out. I might notice performance or security flaws that their own team wouldn't consider in a typical PR.
While I actually enjoyed the style of work where reviewing a PR isn't a chore, there are a couple issues I'd like to see improved. Their rates could be improved for the best engineers. Also, the number of jobs isn't always enough for the number of reviewers. Gig work is much nicer if you can actually choose the hours and have more flexibility.
I think the point is internal and external code reviews are two different beasts - no harm in getting an external kicking to improve the coding practices. However, with nobody having skin in the game to get external code reviews into the codebase, they will largely be ignored as “nice but we have work to do”. How could a product like this (I think I’ve seen a few) solve that human nature problem?
They're truly different beasts, but each has clear value. As but one example, I've seen outsourced apps for financial firms where there were literally hundreds of basic security flaws. Would you trust the same review process that allowed those PRs?
To answer my own question, maybe you could massively overcharge and put a bounty on each review item that gets paid back when they get completed? Is it right to make something like this about money? Would the company or dev see the cash?
Could be an interesting way to make it work and try make a higher/more valuable company from this, i.e. the CRAAS company could keep the bounties if not fixed after say 6 months...
I definitely get what you're saying here, but I think if I was given the choice between an internal reviewer that might glaze over some bad practices, or an external reviewer who will miss stuff like "oh be careful calling that code, there's gotcha X, Y, and Z that you need to think about", I'd take the former every time.
It’s usually possible to see if a certain piece of code allows gotchas or not. Global variables, implicit dependencies, undocumented apis or magic strings to give a few examples. If you have many such, then getting a reviewer calling out those bad practices is even more valuable. Even more valuable that they are external, because often many such smell-patterns are stuck due to some political stalemate or cargo-cult within the team.
Most gotchas are actually carried over from the open source framework you build on top of. Such knowledge is transferable and can’t hurt to get another pair of eyeballs to help you with them, assuming you haven’t spotted them yourself already.
My company has used them before. I thought the same before using them. However I was surprised the level of talent they have. For a super senior dev the context that you need for most projects isn’t as much as you think.
It was a net positive for us. Sped up our developer process given its so hard to hire senior devs right now.
They also have ___domain experts. So say you are using some tech your team isn’t as familiar in. Great to get some extra eyes of that code to check for security issues, potential computation issues etc.
Also they are much more broader than their company name suggests. Think of them as developers as a service. In this hiring environment it’s much needed.
I'm one of those reviewers, and I agree with you about the talent. Not speaking about myself, but the people I've gotten to know and also the ones I have referred.
Each PR gets two reviewers from pullrequest.com, and we get to see each others' comments. One will catch stuff the other misses, and we usually support each other. It's most fascinating when we disagree on something, which so far has always led to a high-quality discussion between the engineers and the reviewers.
I've been working with them for most of 2021, and I can honestly say I'm impressed with the review comments I have seen. It's been nothing but respectful and professional. As a plus, it's made me a better code reviewer at my day job.
I have certainly done this. PullRequest bought the Moonlight developer gig platform. A lot of full-time developers from Moonlight also took on gigs from PullRequest as they've been using the platform to sell their service. I'd guess most reviewers have other jobs or contracts.
One thing to note about our service is that we are not trying to replace your code review process if it is already working well and we strongly agree that knowledge transfer is a very important part of code review. ( We actually have code review metrics as well that help encourage and reward your internal code review process. )
However what we do believe and see on a daily basis is degree that we help supplement the process and help catch many issues as well as inject a unique perspective. Our reviewers are all highly qualified, many are maintainers of popular open source projects or work at top tech companies. Our reviewers also gain context over time similar to a new senior engineer on your team. Reviewers also can share notes with each other to build up a corpus of information for your project over time.
I wonder if, instead of just incremental code reviews, there would also be a way to get a 3rd party review our huge codebase and flag issues (architectural, real legibility -- not just "CC measures") to be dealt with. Then you could keep track of them and burn them down as part of "killing technical debt" goals.
Yes! I am a reviewer for PullRequest and since I've been doing reviews with them I've been involved in at least one major review where I was given the opportunity to review the entire codebase. From there I was asked to give comments, suggestions, design ideas, and general feedback on a number of different criteria.
I'm just a reviewer and can only comment on what I've experienced. But I can say that what you're asking has definitely been done.
> Our reviewers are all highly qualified, many are maintainers of popular open source projects or work at top tech companies.
Isn’t that potentially a huge problem? What if your reviewers work for my company’s competitors? I don’t want them seeing our code base. Do you have any methods to ensure that doesn’t happen?
Conflict of interest is something we take seriously and have processes in place to ensure this doesn't occur. All reviewers aren't able to review or see the reviews from all customers, we have tools in place to facilitate the best matches for both compliance as well as quality and familiarity.
Are you saying your employees will never leave to work for your competitors? I have not worked with this company but most of them needs an employee to sign a strong NDA that protects IP. That must be sufficient in most cases to protect your IP.
I wanted to join as a reviewer, but you don't accept reviewers from outside the US. Is this something what you are working on and has a chance to change in the foreseeable future? I work for a public US tech company but I'm located in Europe.
For the past year, I've been working for a company where there are a lot of extreme novice mistakes in the codebase. Even though they reviewed their code, a novice developer reviewing another novice developer aren't going to catch things that are obvious to a developer with 5+ years experience.
IMO: Target shops where they just don't have the expertise on-hand to do thorough code reviews. Don't waste time trying to convince a team full of experts with deep ___domain knowledge that they need you. (They probably don't.)
We also have a "problem" where there are some components that are a different language than what most of us are experts in, so they end up being developed by a solo developer. When we need to jump in, as we learn the codebase, we also see novice mistakes that are very hard to fix, because we just don't have many years of experience in that language / platform.
Thus, IMO, on your website, list out situations where shops will clearly identify a need for your service. (Team full of novices, solo developers, team members quit.) Don't go trying to convince "everyone" that they need you.
As somebody who's done a bunch of reviews on the platform you realize a bunch of things about development that we don't want to admit. When I review code for my own team (for my day job), there are many times where internal pressures on my time will make me prematurely stamp LGTM on one of my co-workers PRs that I trust. When I'm doing reviews on PullRequest I remove the "trust" marker in my review along with this strange thing called "economic compensation" where I'm paid to spend time working on it, rather than having somebody ding me for not completing some other task that needs to ship this week so I spend more time reviewing the details.
I think there are a few levels of code reviews that should be in place.
* Automated checks eslint/typescript as examples, you would be surprised at how many companies don't have this!
* Best practices -- react hooks, golang interfaces, calling conventions...
* Testing -- are the tests structure to test good and bad, are they brittle?
* Security -- Did you build the right IAM role in terraform, did somebody just checkin their GITHUB key (ok, that should be automated).
* Performance -- Is that Promise.all going to dispatch 1000 calls in parallel.
* Architecture and inter dependancies, there is a limit for a 3rd party here.
Now if you're the Lead/Architect on a project and at a minimum you can outsource the Best practices / Testing portion of the pull request to a 3rd party you now can focus on the architectural dependancies that are really what you care about.
As an architect you can easily provide reviewer notes to the person doing the review that you are interested in focusing on specific areas of improvement across your team. Giving you the coverage to focus on the high level issues and inter dependancies while not focusing on variable names or test coverage.
Your time is valuable, you should spend it on character development not on the punctuation.
I am a PullRequest user/customer. I shared some of your concerns when I first heard about them, and was admittedly the least on board them trying them vs the others on my team. In short, I didn't believe that an outsider could provide adequate reviews and that, at best, an outsider would supplement our internal review process. I was wrong, and have learned several things about code review from this company.
1. Once ramped up, PullRequest provides consistent reviewers for project, even down to fairly granular sections of the codebase. i.e. we’ll get the same reviewer or sets of reviewers who review code for backend architecture changes, a different person (but consistent) who reviews security code even within a monolithic repo. These reviews feel like a real part of your team after a while, but fully focused on providing quality review.
2. It’s true that the reviews are not involved in initial planning conversations, but in practice this turns out to be moot or even a net positive. This is because they are providing a removed perspective on the review. I can think back to one time very specifically when the team planned to implement a feature in a specific way. An engineer went off and did so as had been planned by the team. An internal review from any of the original teammates who had planned the feature with him would have immediately approved the PR since it was exactly to the original spec. However, our PullRequest reviewer caught a MAJOR VULNERABILITY that the feature’s architecture had presented. Thus, a fresh set of eyes from an outsider who knows our codebase but is not involved in planning/implementation discussions was critical. IMO this is one of PullRequest’s greatest value-adds and why I will always advocate using them /other services like them no matter what team (though I don’t know of any comparable services, though I suspect more will arise and 3rd party review becomes table stakes, but that’s a different discussion).
3. The people that PullRequest gets to do reviews are top notch. In some ways, overkill from what would be required to actually develop a feature from soup to nuts, but it gives us more confidence to let junior developers run more freely on larger features knowing that they will have to pass code review from PullRequest.
While I agree in general, some forms of code reviewing require less context than others. They say "We review within your tools to catch security threats, stop crashes, and fix performance issues before they reach production.", and it does seem to me that these things are less a matter of style/context than just noticing potential issues. Not sure though, I had the exact same thoughts as you when first seeing this.
Edit: to be clear, I personally don't see the value here, just playing devil's advocate.
Code the falls between between "I need an experienced team member to see the issue" and "A few static analysis tools would no doubt have seen it" must be vanishingly small.
For specific areas, I can see myself paying for external code review. For example some open source library authors sell it as a service, so they can review the specific parts of the code where the library is used. Rob Menschings FireGiant is one such example.
Security reviews (Audits) I can also see a good use for. The current practice here is already to have external non-___domain-experts review the code. So making that simpler or more frequent is a net win. The reviews I see little use for would be normal feature code, (pull request reviews) which need a lot of context and ___domain knowledge to even begin to review.
Not audits, or reviews of specific areas or aspects of the code.
I suspect that there’s only a tiny, tiny fraction of issues that are above the complexity that a linter and test framework can’t identify, but are below the complexity that requires deep codebase knowledge, and I would say that the overwhelming majority of value of code reviews is inside the issues that require deep codebase knowledge.
Maybe this service could be good to point out superficial security bugs though, but the infrequency of these coupled with the effort of external human engagement I think would be a barrier.
Also a few times in my career I’ve seen a team with no senior devs on it, they usually fail due to inexperience, maybe this service could help a team like this - who are producing a lot of really obvious mistakes …
What will actually happen, is people will use these kinds of "context-free code review" services, then say a code review has been done. Technically correct in a narrow way, but not what most people would expect.
This isn't quite true because in most real world scenarios, one of the requirements for code to be correct is that it provably halts in a fairly limited amount of time.
Not at all, it is saying that if the only things this service is providing is a human doing SA (since they dont have any deep-knowledge of the codebase), then you could just use an SA.
Code review in the real world misses the Halting problem, the only way you can really see it is if you know the codebase well enough to SEE it, and any 3rd party that is given a pull request to complete in a timely manner will not have time to fully learn your code base or even the modules you are submitting.
That was my initial reaction too, but then I thought "using such a service might encourage the code-based to be external-reviewer friendly?". That is, instead of all the shared history/context that internal reviewers have, make all that as explicit as possible, ideally in the code, or at least in good code comments. Would also go a long way to avoid the "bus factor" (or more commmon: the "sudden-quit factor").
I guess commenter would agree with you. They're adding a point that, on top of what you said, hiring a new dev (more likely and frequent in many cases than a sudden quit) would also trigger pains similar to a sudden quit.
Code should be written to be understood without that context. If comments and documentation aren’t enough context for the code to be understood, it probably isn’t written very well.
I'm talking about code where thee context is donain knowdledge and architecture.
Reviewing things like style, performance, security, framework best practices etc is pretty easy work and rarely the bottleneck in a team in my experience.
Basically: any kind of review where you could comment on a single file only, is easy. The important and difficult part of review is "Is this the right thing to do at all? Is it implemented using the right approach to begin with? Do we have other functionality that already does this? Does that other functionality use the same approach or is there good reason for this being different? Does this follow the business logic properly or are there any signs of misunderstanding the requirements? Are the requirements sensible?"
I'll argue that, in an ideal world, most of the questions you're asking here should be addressed before anyone writes code.
A basic spec, with "here's the idea", "here's how I plan to prove the concept viable", and a rough plan of "here are the software components I'll use" doesn't take long to put together, relative to actually coding the thing up.
Having that document and getting it reviewed should answer a lot of those questions before code is committed to paper.
> in an ideal world, most of the questions you're asking here should be addressed before anyone writes code.
I agree completely. But code review to me is the chance to pick up on those situations where the situation wasn’t ideal. And even if this is just one time of 100, that review was still more important than the remaining 100 “normal” reviews with more mundane feedback.
I agree with your sentiments here. A specific, trivial example of this would be a distributed systems architecture where mutex locks are being employed (or really any distributed structures like queues, pub/sub, etc).
Trying to review a PR for a single service that is interacting with locks across a dozen or more other services would be fraught with assumptions and missing context.
You could make the claim that if documentation is perfect, that makes the situation better for the reviewer. But this is neither a practical expectation, nor does it completely mitigate the problem.
EDIT: forgot to note that this is where bottlenecks in review are, in my experience. Not in the first-order review of syntax and semantics in the single file being reviewed.
It is very bold statement.
Code models some real-world entities (___domain). Code (completely with comments and documentation) cannot and should not document fully ___domain. It is context, which is needed to understand code.
Yes, simple CRUD application can have all context encapsulated, but what's about some code which models, say, some aspect of chemistry? Should this code have enough context which includes several post-grad university courses?
Or «simpler» example from my current $Job: we have a lot of code to build some models of derivative stock exchange instruments (options, futures, etc). Enough context for this code is, like, full shelf of 1000 page books. Good luck to review this code for everything but off-by-one errors if you don't work in this area for 5+ years.
Your example is, IMO, the exact use of this service. If you're a chemistry expert, you're probably not a coding one. These reviewers will ensure your code is testable and likely to do what you hope it does, in a way where your fellow experts can read and write their own automated proofs (tests).
I think I might be onto something here. "Full time developers as a service"