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 mentioned this idea to a very knowledgeable enterprise architect. I was mentioning how much I'd enjoyed being able to use the code review process in this way. His reply: As good of an idea that is, it breaks Agile. So long as the code meets the biz-provided spec, it must be accepted, and if there are other concerns with the code, to make a tech debt ticket to address it at a later date.
His logic would apply to tests of all sorts. As long as the dev says the feature is done, that’s the end. And he’s wrong. As teams (or companies, or whatever unit) we get to decide our definition of “Done”. I suggest that definition include appropriate testing. And appropriate code review, which serves two purposes… catching defects and knowledge sharing.
In his world, testing the software belongs to a different team than the implementation. Differently managed, differently staffed. Not with engineers, but with testers. It's difficult to say the least to move from testing into engineering
And you can't just redefine Agile like that. It's the businesses' money, culture, and productive means. Not yours. If they want teetering software stacks with no thought given to maintainability, managed by non-tech-savvy staff, then that's what their money will buy.
We enjoy an environment catering to our needs because our orgs can afford to throw a lot more resources at better managers and better talent. As a result individual contributors can contribute not just tickets, but also to help improve the way we work. Not possible in heavily top-down org structures.
Ugh. I haven’t worked with an independent QA team in 15 years or so; testing is the responsibility of the dev team and there are test engineers on each team to facilitate that.
Maybe this is a difference of building a product vs contracting? We have to keep what we build running for years; there is no turnover to a client where can declare the system “done”.
As for redefining agile, I’ve done no such thing. The agile manifesto calls for working software. It calls for collaboration. Avoiding knowledge sharing amongst team members and throwing software over the fence to a remote test team are both counter to that goal (in my experience).
In my experience, splitting the testing team out of development materializes refactoring as costly.
"What parts of the system did this impact?"
"Well, this is a core piece of our ORM config, so... the entire data layer, so from a functional perspective this change impacts every part of the system"
"Then you're not changing it, QA can't rerun all their old tests, it would take months, most of those tests don't even work anymore".
End result: refactoring never happens. Get it right the first time.
Naturally. In an Agile environment it's more or less impossible to get it right the first time without seriously forward-thinking architecture, which takes time that could be spent pushing harder towards the deadline.
It's a business decision to organize software production this way. Refactoring and ease of software maintenance are quality of life issues for engineers. Not business concerns.
That's how a managerial bureaucracy approaches the problem of deadlines: The second it looks like it's working, mark it done and move to the next task, and deal with the bugs later - ideally the manager will move to another position by then due to their exceptional productivity in meeting the set goals, and the bugs will be someone else's problem to solve...
Microsoft famously called that process “infinite defects methodology”, and decided it was a bad idea in 1989. "In general, the longer you wait before fixing a bug, the costlier (in time and money) it is to fix."
Anyone with Architect in their title receives skepticism from me. I've turned down title changes that include it and I refuse to put it on my LinkedIn.
I think that attitude is a little short-sighted, and perhaps a little bit of "anyone that isn't a pure developer is incompetent" elitism.
For one thing, titles are just titles, and they stick around for historic reasons. For example, there really isn't a great reason for companies to call people DevOps Engineer, but it still happens.
I'll take the Solutions Architect role as an example. It's basically sales or customer-oriented developer or technical resource that works with customers to determine what a solution to a problem will look like. "Architect" is mostly meaningless in the sense that Solutions Architects don't really architect much of anything. Usually, they just come up with a plausible path forward and visualize that solution to all the stakeholders involved. This includes travel to customer sites, something that developers are basically never willing or expected to do.
They're the folks who deal with the god damn customers so the engineers don't have to. They have people skills, they're good at dealing with people. Yeah, I mean, considering the staff engineer on my team does not shower, there is value in that role.
Most of the value in the Solutions Architect is how they're able to work with customers to discover their needs on a more technical level rather than at a high level. Once the plan is determined, the solutions architects don't actually build the solution on their own, they're more like an interface or leader to the development team that builds the solution.
I don't want to put words into your mouth, but maybe you're skeptical of "architects" because they aren't typically expert specialists in one small area. Maybe that's why you don't want to be associate with them. Understandable, perhaps, but don't be misguided into thinking that "architects" aren't skilled professionals who add value to the company.
On top of that, I believe they're often paid higher than developers ;-)
His world seemed completely and utterly divorced from mine. He was deeply connected with big enterprises and how they manage to meet business objectives regardless of not being software engineers themselves. In our world, we take for granted that our managers and leadership are technically proficient. In his, the "Engineering Manager" is a rarity, the people managing engineers are just ordinary managers.
Software architecture, in this world, is how you escape the rat race of soulless ticket punching without having to go into management. You use your skills and experience in an advisory role. Obviously there are better or worse architects, I've had the pleasure of working with really good ones. But it often feels like a title and field borne out of a need to retain top talent. (read, not push them away to competitors)
The case for code review is fundamentally economical: it saves the organization money by finding costly issues earlier when they are cheaper to fix without imposing a costlier burden for the review process itself.
As generic "knowledge transfer", or even increasing the bus factor, I would disagree. The best knowledge transfer experiences I've had have been dedicated meetings/workgroups dedicated to that purpose, and they tend to encompass larger scopes than a single commit/pull request. I've also seen the difference in devs who contribute to an area having previously only been reviewers of code in that area, vs. actually having a knowledge transfer session with that area's lead beforehand, and in the latter case they are more effective (actually even if they had never reviewed code in that area before, as was the case with interns or new hires). To me knowledge transfer is the nice possible side effect, but not the primary purpose.
Few developers and software project managers would argue that these are direct benefits of conducting code reviews:
• Improved code quality
• Fewer defects in code
• Improved communication about code content
• Education of junior programmers
And these indirect benefits are byproducts of code review:
• Shorter development/test cycles
• Reduced impact on technical support
• More customer satisfaction
• More maintainable code
Maybe for you. For others it can have all or any of those other things as its primary purpose. One important thing for me is that it prevents or at least mitigates unilateral insider attacks by having two people required to change code.
Having a bus factor>1 is still a major prerequisite for doing that. Because if you have a junior developer review the changes of a senior developer, there are so many social engineering tricks to make the backdoor pass the review that it isn't even funny.
Personally I work on a 2 person team, the vast majority of my reviews either have no changes or "hey maybe this thing should be named something else so its more consistent".
For us the peer review is almost completely for knowledge transfer. Sure we know what each other is working on, but we still have to maintain each others code if something goes wrong and the other is not available.
So I agree with this 100%. Even in bigger organizations where the peer review was more formal, I feel like that is still the primary goal.
I do fail to see the benefit of this, especially looking at the price of it. Outside of maybe scripts? I can't really see how much they can actually help without the context of the larger application. Without that context can they really provide more benefit than AWS CodeGuru (or similar) could offer?
> Say it with me: Code review is a knowledge transfer exercise.
It is, but (say it with me?): Things change.
Code review is not the sole knowledge transfer exercise, nor should it be forever. You could say similar things about "make format". Now that our formatting is automatic, we can discuss code at a higher level. If code reviews were standardized or even automated, we could discuss it at an even higher level.
In most of the projects I have reviewed for pullrequest.com, the engineering team is also doing its own reviews. We are "another set of eyes" as it were. Many of our larger reviews can end up becoming lengthy conversations between us and the team. There is definitely a lot of knowledge transfer going on. What's been truly rewarding is when someone on the team comes back to us and asks for advice on the best way to solve a particular problem.
Even without hard stats and evidence, I guarantee most people don't think of code review as mostly being about KT. The purpose(s) of code review commonly contain the ones you described, but it varies from team to team. The most common I'd guess is putting a 2nd pair of eyes on your code, checking code quality and finding issues.
Finding bugs is of questionable use because it's sort of limited to obvious bloopers. One usually needs to be deeply involved and familiar with the business logic to be able to spot a real bug of the type that will give you serious trouble - external services obviously will never have time for such commitment.
That said, I think there's still a lot of space where this service can be very useful: from improving your code style and an affordable way to have security audits, to just a support net for developers who might feel overwhelmed by a task sometimes, and could use some friendly advices from a seasoned dev. It being an external service can also make it less stressful and personal, which is great as some devs see code reviews as a criticism of their skills and go all defensive about it, creating tensions in teams (sounds silly, but I've seen a lot of it).
Some companies doesn't have enough people to review the code, at least not with the required seniority. This way there is also knowledge transfer, but to the company. And with people not in your payroll that can provide objective comments since they are not afraid of telling the wrong person that their code sucks.
This. Code review is a terrible way to find bugs... At best you'll get the more Senior reviewer to spot library uses that are known to be workable but problematic (eg poor performance), coming from their experience. But straight up logic bugs are hard to spot.
The big thing that code review achieves is that it ensures a 2nd person understands the code that was written, and therefore it is possible for the reader to understand.
So 3 years down the road and you're looking at some counterintuitive piece of code, the reader isn't wondering *why on earth did he write it like this? Is it working around some cryptic edge-case bug in the framework or were they just stupid?"
But: When you have a solo developer working on a component in a language that you aren't familiar with, team full of novices, component delivered by a contractor...
i thought code review was to assert dominance and to block as long as possible for yak shaving reasons; But I do like your version of it! Happy holidays~
Having on-demand engineers look at code, without broader context on the project it is in my view the same as something that can be automated either or both via static analysis and custom ci/cd workflow checks. This can probably makes sense on a project with more junior engineers where many basic improvements can be supposedly suggested without needing broader context? I would be keen on hearing the use case
I can think of one real-world use-case, though I have never used this product.
It's pretty common in larger companies to have legacy products with a couple of very senior (usually overworked) people and a larger number of junior people. The juniors very often have little experience with the language, much less the frameworks, of these legacy systems. So PR's from junior devs are often very frustrating for senior devs (wrong language conventions, not how AWS is done, paradigm misunderstandings, inaccurate comments) -- and the perverse incentives resulting from that are pretty obvious. Bad code ships.
If passing "code review as a service" were a requirement before the juniors could put up changes for seniors to review, in my experience that would be money well spent.
A lot of people use ESLint and Prettier. If you look at the roadmap/issues for those products you'll see that some feature requests cannot be programed, but can be understood without knowing the full context of the business and codebase.
At the same time, if you are fixing a critical bug and you use enum instead of boolean, who cares, just push the fix, but it doesn't hurt to have a gentle reminder show up in your github PR that it is an option to use boolean instead.
In a decent language enumerated types are strictly better than booleans because they have names. Closed and Open, or Paid and Outstanding, or Safe and Dangerous are better than true and false unless you really meant true and false.
my_bills(Paid) avoids the step of reading the function prototype to find out what my_bills(true) means.
In some languages you can make piecemeal changes to upgrade from booleans because the language is happy to silently coerce between a two state Boolean and a two state enumerated type. This is probably bad news for correctness because it means my_bills(Open) might not even raise a warning but it's convenient.
I have reviewed for pullrequest and you’d be surprised how many things one can fix. Why do you think that you don’t have context? You can look at the whole project and see what’s up.
Variable naming is a really obvious example of "you can't write programs" to assess this. Because it needs some general intelligence.
Are very short names OK? Generally not, but x is a perfectly good name for an x coordinate in a graphing application for example.
On the other hand methods named colour (to get the shade) and shade (to get the colour) need some serious documentary explanation of what the hell you're up to even though in themselves they're acceptable names.
Language idioms, and (if this service is expensive enough) per-project idioms are not usually or reliably machine checkable.
Also beginners make lots of confusing mistakes, a program may end up missing the woods (e.g. this should just be an iterator, 90% of the code is mechanics that are doing what the language's built-in iterators do) for the trees (the variable names used for all the counters being tracked are bad)
It's a rare case indeed where I'd be willing to let a third party in on business logic plus the whole source code for something I was developing. I love talking shop but if I'm really having a problem with a structural issue, it's hard to imagine someone outside giving any better advice than people inside who understand the lay of the land. Meanwhile, it's a pretty much totally unacceptable security risk. So what's the upshot to this over an internal code review?
Well, maybe the people you have available to review code aren't all that good. Nobody ever wants to admit that their team is below average, but 50% of teams are.
Maybe due to limitations on team size, it's not possible to have an in-house expert on every technology used.
I used this service at my previous employer to launch a small django/nextjs app and it was mostly fantastic.
Background is that I worked at an VC-backed startup as a dev after doing General Assembly’s full stack bootcamp. Left that job to do ops/growth, and ~2.5 years later volunteered to build the web app when my company put the project on their roadmap.
As the only developer at the company, pullrequest was great for:
- a general gut check on how I was doing
- recommendations on how to better write js/python. linters help but nice to have a person offer feedback on more advanced ways of doing things
- sourcing documentation on best practices. I found a lot of typescript/JavaScript resources to be inconsistent/confusing. Was great for someone to find and vet guides for me.
- help with bugs/errors
- basic library choices and architecture decisions
It was also fantastic to have several people reviewing my code at once. Gave me a perspective on the type of engineering manager I’d want to work for. Some folks focused more on technical details but struggled explain their changes in plain English, while others seemed to be the other way around.
pullrequest was not great for:
- doing things fast. They didn’t have a real-time messaging feature so I’d get hung up on waiting for feedback. They do have a 24(?) hour turnaround, but when several folks are commenting on the same PR it gets hard to track what changes _really_ matter vs what they are throwing out there as a nice to have.
- Anything that required context outside of the files committed. Though with some extra long PR comments I could manage.
I would 100% recommend pullrequest for small teams who are heavy on more junior devs or migrating to a new stack. It is an inexpensive way to ramp up learning.
Last thing here — when I worked at the startup my code was rarely reviewed, and I’d have to actively ask for it. Not all companies follow best practices (even if they have the resources to). I would’ve loved this at my past job, too.
I will admit that I thought this was a joke on the "SaaS" everything trend.
I can see how this works for fairly limited web applications for example, but as soon as the application grows in complexity and interacts within a bigger system of systems, I am doubtful that it would be logistically possible to outsource the code review (legally, knowledge transfer wise, and a plethora of other angles I'm a tad bit lazy to consider).
Overall, why not, if it's priced correctly, then it's probably a set of additional eyes for small projects. But for anything medium or higher, yeah, I don't see this realistically working.
Maybe OP (?) can explain if I'm wrong (very likely). There's probably something I'm missing.
Overall, for bigger and more complicated systems you would have an architecture that is peer reviewed and communicated internally.
Afterwards you have individual components that have their code reviews and still need to follow industry best practices. I think external code reviews is great idea as it could allow the team to focus more on conceptual reviews and consequences to other systems.
I've yet to see a company ask for feedback once they were done pushing out a half baked API. And if anyone uses it, they do get plenty of feedback. As far as whether the code is internally badly written... if the guy writing the code can understand the suggestion, he's probably already done it or decided why not to do it. If he can't understand the suggestion, then it's a waste of money.
:wave: Pull Request reviewer here with over 1000 reviews.
I've reviewed code across languages, team size and maturity. A good static analysis tool is not code review. The word 'context' came up 37 times in this thread (by the time I hit submit) and it is worth digging in to how I build and maintain context with teams. First up, it is my responsibility to uphold, not define a team's best current practices. If you believe a linter and static analysis tools are best current practices, you probably do not need code review. The code review process is an exercise to affirm how well a pull request meets the expectations (context) of a team and suggest remediation where appropriate.
I assume 'context' used in the threads to mean "how we do things here". As a reviewer, I conduct review understanding the problems the team wants its code reviews to optimize for or to avoid. This is due to the people creating the platform. It's a solid platform for reviewers to get things done. Every line of contributed code I review is done with an eye towards ensuring it affirms a team's stated objectives. If those objectives somehow falls outside of what I know to be true from my experiences as a professional and best current practices (BCP), I am empowered to engage the team.
I've had teams request to never provide guidance for coding style issues. Others want to know if how they modeled a React component tree could be improved. My success on the platform relies on always building context. Without that rapport it limits the depth of the review. Because code reviews are interactive, they tend to get better over time. When things go well, the relationship between team and reviewer is seeking a pareto optimum between the pull request and the team's best current practices. When needs change I adapt my reviews. It is the same treatment if you're a one person shop, SME or a listed company.
I thought of context to be more about domain/tribal knowledge rather than just "how we do things here".
You can be a ruby expert, but to review a ruby PR for Stripe's backend I think you'd need to know a lot about the various internal systems, downstream users, etc that the code impacts
I've been a reviewer on PR for a couple of months now and it's been amazing. I've gotten to help dozens, maybe hundreds of developers improved their code, fix security, readability, testing and lots of other issues. Sometimes the PR is short and the review is short but sometimes it's much more involved and I've had the opportunity to really explain things and educate people in a way that no lint checker could. I'm able to apply the wealth of my experience and knowledge with specific guidance based on things I've learned from building software for 22 years. I can't think of another way that a company could gain access to that kind of guidance for only $699 a month.
I can go into detail explaining how to safely and reliably structure something and the feedback I get from the developers is positive and appreciative. I also collaborate with other reviewers about things we're seeing and I've learned a ton in the process because the other reviewers they've all got such a depth of experience and knowledge in different areas.
I don't do reviews full time, I have a day job, but I've been earning about $1000 a week doing it just some nights and weekends and that has been really great too.
Overall, I think PullRequest is an enormously positive thing for its customers and the reviewers. We're not trying to replace people's existing review processes if they have them. Our goal is just to add to our customers' capabilities and I think we're going very well at that.
Saw this link here in the past. Didn’t pickup. The reason why I don’t like it is that random people, regardless their expertise, cannot just review PRs and understand the impact of the change on the system without being deeply involved in the product.
Nor can arbitrary people weigh the balance between accepting technical debt and shipping a feature.
It involves a risk / benefit analysis.
On the other hand, maybe you can build a relationship with contractors that focus on reviewing code. And maybe it can give insights your team won't have?
I'm a little more positive about this. A careful code reviewer might be able to spot generic security or even logic flaws in code, such as inappropriate use of strcpy in a C program or code which is unreachable in a way that cannot be detected by the compiler. Also there's a lot of scope for automation, such as running Coverity or other free and commercial linters/checkers, although also a danger of overwhelming the results with false positives and junk.
Results from these tools typically go to the developers, not to the PR reviewers. They see only the result of fixing a vulnerability, and if there is a new vulnerability introduced, these static code analysis tools should detected them before deployment to dev/prod.
I'm currently the only developer in my startup and I crave for high quality feedback, so this service sounds like it's a gift sent from heaven – especially for the $699 price tag per month. I guess they probably make a mixed calculation: take a small company with 10 devs. Two of them are part-time, on any given week one of them is on holidays/sick leave, one is very junior and doesn't contribute much yet, another one is more concerned with project management than with writing code and one has in fact transitioned into management but remains on the dev team because it's part of their identity to still "get their hands dirty" (even though they don't have the time to produce much code) – so you might end up with sth like 4-7x more code than I write, but 10x the payments. My productivity is not above average, but I have very little overhead besides coding.
Unfortunately, I cannot find any information about a minimum team size for the PRO plan. I think I'll give them a try.
I have performed reviews on pullrequest.com, and lack of context tends not to be an issue. Code review is an interactive process, where questions can be posed to the PR authors, and PR reviewers have access to search the codebase. Customer success at pullrequest.com also provide context for the repo and organization working practices, attached at the top of PR descriptions, to help reviewers with context and to know what kinds of feedback are useful. Sast and Dast are still not sophisticated enough or widely used enough to be good enough. In-house senior and principal engineers often do not have the time to be on top of every repo and every PR in their organization, and need to outsource an extra pair of eyes. It is highly common that I am reviewing codebases that do not have any Sast or Dast, or have workarounds to silence the tools, resulting in bugs and security issues slipping through. It is very rare that I am able to approve PRs without comments, even after an internal review has taken place!
> Code review is an interactive process, where questions can be posed to the PR authors, and PR reviewers have access to search the codebase.
I also performed reviews on pullrequest.com (~2+ years ago?). I firmly believe code reviews are an interactive and knowledge sharing process but was explicitly told NOT to ask questions during pull requests, but instead to make statements focusing on bugs and style.
Based on the comments here, this def looks valuable to some of pullrequests.com customers. I believe this is more of a human acting as a safety net, or risk mitigation strategy, and loses the majority of the value that pull requests can provide to an organization.
Just curious about the flavor of code you were reviewing. Can you elaborate a bit on that? Specifically - any software integrations with hardware? E.g. maybe a distributed cloud service that talks to on-prem firmware or similar (think IoT, cisco/juniper network gear, etc).
There is a whole class of software that I can't imagine someone could come in blind to and offer any real value to necessitate the balance sheet exposure. But I'm very interested to hear if folks there have had to do this (yourself or others).
I'm a reviewer and a firm believer in PullRequest. I do believe that some of the concerns expressed here are valid but I think they belie a fundamental misunderstanding of "what you're getting" from PR. Having an under-experienced contractor review your code without context, pointing out things that static analysis could/would catch sounds like a terrible proposition, but that is NOT what PR provides.
Speaking from personal experience, the reviewers are very knowledgable professionals who are not doing this for money as the primary motivation. The pay is great but is more of a perk/reward for doing something we enjoy, and doing a great job at it. The staff at PR does an amazing job of coaching and guiding reviewers on how to provide the most value to the client and also encouraging us to do our best work. It is never anything like "You need to be faster and bill less, do more, etc" it is quite the opposite, we are encouraged to "keep the clock going" while we research, gain context, etc.
Everything is focused on providing value to the client, and IMHO a stellar job is done. Every review comes with detailed information on who the client is, the make up of their team (seniority, etc), recommendations on what to look for (and what NOT to look for) etc. Essentially you are putting your code in front of people who have been there, done that for a long time and are acutely aware of not only risk but also pain points and how to avoid tech debt. We are providing insight, recommendations and even code snippets on how to avoid repetition, speed things up, or make them easier to maintain.
I understand NDAs are a thing, but I still don't think I'm too comfortable with the idea of letting a bunch of third-party people look at a bunch of my internal information. And I understand this is targeting startups a lot more than large established companies. But to me that's even more concerning because as a startup you're really trying to move fast and hoping someone doesn't beat you to the punch, and you're handing a bunch of people you don't know how much of your secret internal information, and hope that they don't go talking to other people about it.
I think most engineers (and execs) overvalue secrecy.
My limited experience is that access to your code is really not that valuable to competitors. Code might be valuable to hackers wishing to targeting you, presuming your business is a valuable target.
Most startups are too busy finding product market fit than trying to worry about sharing 'secret information' (your actual secrets shouldn't be in your repo anyways).
I see a lot of comments regarding lacking context/knowledge of the full system/integration. I get that, but I think there is real value here. I have absolutely worked on teams where they were fire fighting for months at a time, where there were 1-2 developers total, and just not enough time or resources to do proper code reviews. While this service could miss big picture things, it could absolutely catch low-mid tier issues.
My biggest issue would be from a security perspective. The background checks and everything are nice, but there are some systems I would just never let this service touch.
Overall, this is interesting! Wish the team behind it the best of luck.
This implies that you have no-one in your team to do code reviews and that you're fine allowing random third parties access to your infrastructure and a peek into your projects and code base... Both the premise and the proposed solution sound very odd to me.
Most projects I have reviewed for pullrequest.com have reviewers on the engineering team also. In a way, we are helping them get better at reviewing their own code. I imagine that some teams won't need us after a while. On the other hand, in some projects we have become trusted members of their teams.
I'm not sure this is a good idea for enterprise startups building closed-sourced software. As a software architect, some things you just don't delegate. Code Reiews are some of the most important things I do. But (and please don't downvote me for this, I know there's an instant reaction to downvote anything blockchain related) for DAO's this would be amazing.
A DAO (Decentralized Autonomous Organization) might be running a software as a service. But it might not have any full time employees. It might not have any employees at all. A lot of updates to the software might come from random people (or bots). The DAO will need to evaluate and pay for any of those updates before it decides to merge the pull request. A code review as a service would be an absolutely invaluable tool for a DAO with a software product that doesn't have any full time architects to perform the service.
I didn't downvote you, but I'd love to hear some situations in which this hypothetical DAO would be comfortable running code that it (where "it" means "an engineer who has a stake in the DAO and so is invested in its success") had not reviewed? How would it trust the results of this review? I'm trying to remain open-minded and curious about blockchain-related use-cases.
I wanted to sign up as a reviewer but it seems like one needs a linkedin account to do so. Since I don't have a linkedin account it seems like there is no other chance?
On the one hand, lacking broader context might make this service seem silly.
However I think it can help bust groupthink
It’s still valuable to get more general feedback and ideas explicitly without context to challenge our attachment to current practices in existing codebase schools.
It’s nice to get a pair of eyes with a completely different background give feedback. With context we may have blinders on. A fresh perspective might uncover things we haven’t thought of and bust groupthink
Even though I’ve been coding in Python for years, I still might not have awareness of the best most effective way to do something in general. Imagine all the projects started in the last few years from people learning Rust for the first time?
It may not make sense for the largest, most complex code bases, but I can see it valuable for medium to small projects to get outside perspective.
I'm imagining that to make such service profitable, offering something like $699
per developer per month, you are not hiring reviewers from USA, right?
> Reviewers earn anywhere between $50 and over $3,000/week. Earnings are based largely on the amount of time spent reviewing on the platform and the type of code being reviewed. PullRequest’s payment rates are comparable to those of a senior-level engineer based in the US.
> PullRequest issues weekly payments based on review activity during the preceding week. The time that you spend reviewing is tracked through our platform; reviewers are not required to log hours or invoice.
Let me try: This looks way too cheap to be able to afford good enough reviewers to be worth using; if someone is good enough to be able to pick up a new codebase and usefully review changes that quickly, they'd get a better paying job.
I've read a bunch of the reviews and they seem kinda sketchy - no one will say how much they actually make in an hour and it's a lot of general "no trust us it's good". I can't find anyone saying "I'm making X per hour reviewing Y", if they pay over $30/hr I'm interested but really for less.
The pricing on the site is also super confusing - it's $200 for one hour of reviewing, but $700 for a month? In a typical month at my job I'm doing way more than 3.5 hours of review and also doing a bunch of other stuff on the side. Then if the $700 rate is supposed to be ~120 hours then that's only $5.83/hr which isn't even minimum wage where I am. It is however on par with a lot of gig-work jobs, which makes this even more concerning.
If anyone can say what they made I think that would do a lot to quell all the people that don't trust this. I'm also sure some people must have had a bad experience on the site and I haven't seen that yet which is suspicious.
If anyone from the site happens to see this then I think you should add a breakdown of the percentage of pay going to the reviewers, or just some examples like "For C++ you can expect $40-30/hr, JS is $35-25/hr, etc"
This is always the stat I want from gig-work jobs and never the one they want to show. For example Uber will often tout paying upwards of $25-30/hr, but the actual average is closer to $11/hr, which is below minimum wage in many places, doesn't include any benefits/sick days/etc, and also doesn't include expenses from car insurance/gas/maintenance.
I'd really appreciate if all companies were required to report some basics stats on pay - total employees, min/max, average, and mean would be great
Quit perpetuating the tying of these basic things to the standard employment model. Get 'employers' out of the business of managing access to health care for employees.
Let people 'pay' for their own vacation.
You can provide 'profit sharing' to non-employees.
Give me $3k/week and let me manage this myself vs giving me $2.5k/week and telling me how awesome my 'health insurance' is. I want my access to health care impacted by as few third parties as possible - adding in employers to the mix is completely the wrong direction.
I'd agree with the "pay your own vacation" thing as long as there is profit sharing to ensure people are still getting the income they deserve from the work they already put it to build the company.
But with healthcare, at least in the US, the big issue with pricing is within the healthcare system - not employers paying for it. It certainly doesn't help to have healthcare tied to employment but having people pay high prices themselves instead of a company paying it doesn't really solve an issue it just moves it somewhere else.
It seems like the most proven solution is to socialize medical costs more, but that seems like a long-shot if we continue to insist that healthcare has to be profitable in the short-term. It's like saying "no we won't build this road because we can't charge the drivers tomorrow to make a profit on it", totally overlooking that it's an infrastructure investment and not a purchase
That would be lovely, but pelasco's point is still a good one. If PullRequest _only_ pays "rates[...]comparable to those of a senior-level engineer", without those extra perks (and, to be clear, I agree with you that in an ideal world those perks would not related to employment), then PR's actual total comp is actually effectively much less than for senior-level positions.
So, we can infer that higher-skilled individuals will take the more highly-paying positions, and that the folks working at PR will be less-skilled or juniors. That's a gross over-simplification, of course, but it probably bears consideration.
This tells me that the pricing model is likely based on a previous usage study. They must have found that the average user requests less than 2.5hrs of code reviews per month. Otherwise, the pricing doesn’t make sense to me.
For many of us who work for pullrequest.com, it's a side gig. Some are full-time, but I have a day job. I like to do 1-2 PRs per day, and it pays for my iPhone and MacBook habit.
Gosh, building any tools that target developers as customers is a gargantuan task. Kudos to you for attempting something new here and giving it a whirl. We developers are some of the harshest critics around, no matter how legit or unfounded our criticism may be. Keep it up!
I really love this idea. It's like static code analysis but with more specific feedback based on context. This could dramatically improve code written by junior and middle level engineers. I'd use it!
The only stumbling block seems to be that a lot of devs seem to be resistant to it. I'm not sure why that is; there's many forms of code review and feedback, from shallow to very deep. And I've seen many teams that fail to do proper code review. This could be an excellent introduction to proper practice for immature teams.
It is new to me, but raises a concern that they could only review for general engineering quality, not whether or not a PR is appropriate to a codebase. How would they know if the same problem has already been solved elsewhere, that no wheels are being re-invented, or that a PR is stepping on the toes of something else the team is working on.
Because that is the value I've found in code reviews - not generic "is this code elegant?", but "does this code play well with what everyone else is doing?"
I guess you could first have an internal architect or similar vet the PR before handing the PR to this service for the "technical details".
As you said, the big value in reviews are the points you mentioned. Correctness/technical quality certainly has value, but at $700/dev/month the reviews better be really good. Especially since doing it internally has value as well (knowledge sharing in particular).
I lot of people are saying CR is a way to transfer knowledge within a team. I think they're missing the point. Sure, you can transfer knowledge within a PR but don't you think that's a bit late? By the time CR is requested, the code has been written, the architecture has been thought of and so much time has been wasted doing the "wrong" thing. It's a horrible way to learn/train.
Good code reviews are superb for helping teams accelerate into new technology areas, frameworks, languages, and integrations.
As a top of mind example, when a team wants to spike on migrating from C++ to Go or Rust, including using libraries and porting services, then I see very high value in paying for skilled contractors to do code reviews-- because what your team is gaining on-demand upskilling.
“Please don't post insinuations about astroturfing, shilling, brigading, foreign agents and the like. It degrades discussion and is usually mistaken. If you're worried about abuse, email hn@ycombinator.com and we'll look at the data.” - see https://news.ycombinator.com/newsguidelines.html
Yes, you're right. I found this example pretty blatant and egregious, and my annoyance with it got the better of me. I should have handled this differently, thank you for the reminder.
I'm a reviewer on PullRequest, and thought I'd share some perspective. Happy to answer any questions in comments.
Background - I've built and led engineering teams at multiple fast-growing startups. In doing so I've seen the incredible value PR's can provide, but also the huge cost of them on small teams. I review on PullRequest part-time as I work full-time building a startup.
Many of the critics here are right. The PullRequest service won't catch every bug. As reviewers on this service, we lack context* to fully understand the impact of every code change.
However, this can be a blessing in disguise. As an outsider, I bring an entirely different set of context to the project. I can see errors or improvements that teams have become blind to, I don't have the pressure of shipping for X release, and I've often been not only where these teams are, but where these teams want to be in X months.
This is all done without using any man-hours on the client team - which is often a critically short resource.
Ultimately the proof is in the pudding, I raise comments on almost every single review I do, raising from best practice to architectural to security vulnerability, and the majority of the time teams take that feedback onboard.
Other QA:
Where are you located? I'm located in San Francisco.
Who approves PR's? I tend to consider my role to advise and sometimes mentor. I will give opinions, but it ultimately the client's job to approve/reject PR's.
My code is great, I don’t need this! Have you tried it? As an outsider, I catch peoples blind-spots. That said, PR is always looking for great reviewers to join the team!
* It's worth noting, that between seeing the code, optionally having access to the full code base, and asking questions of developers - I develop a decent mental model of most projects.
Here's a question: do you get pushback about "best practices"? (Sometimes really just "standard practices".) If so, which ones in particular? And do you actually continue trying to explain why it's a good idea, or just throw up your hands with a feeling of "I gave my advice, they can ignore it at their peril".
Having actually convinced people of the value of certain practices (like favoring fast focused unit tests over slow broad integration tests that happen to require the full system to be running), which leads to smoother and more succinct reviews in the future -- like "please add tests" doesn't even need to be said if the developer values them and already wrote some -- I don't know if I'd put up with the stress of having to fight the same battles week after week with a new group. Though I suppose it's possible that you might get good at convincing people of a certain thing, and the ease of convincing can reveal whether the thing is really closer to "best" or just "standard".
I've gotten pushback on best practises, especially from more junior developers or developers on a tight deadline.
I do C# and one common, yet trivial example is poor naming conventions.
As a reviewer we have certain tools we can use to encourage change:
* We can make comments low priority, so the advice is there, but it's skippable.
* We can make summary comments, that are opinionated but don't expect any immediate or direct resolution (great for architectural thoughts).
* We can include example code snippets in our comments. This reduces the burden for the developer to adopt a certain change. I've even gone to the extent of writing small programs to validate a refactor or prove an error exists.
* Sometimes it's not an error, but how that company wants X done. We take note of these for future reviewers.
Overall, this is a diplomacy game, the only power we have is soft power. I find it good practise, as I'm typically a direct kinda guy.
Results?
I've seen a lot of developers, after being introduced to new ways of working, implement that on later PR's. Sometimes immediately, sometimes after it arises a few times. Examples include improved naming, better code structure, usage of newer language features, more clarity in the code, or better SQL injection protection!
Thanks for the reply! It's nice to hear that a lot of devs are receptive to change and aren't treating the service hostilely. I like that there's categorization with expectations encoded in the category, I can see it helping a lot of things. A huge chunk of my reviews used to be in Code Collab, which... lacked certain desirable things. (But it got right a few others, so using it wasn't a totally bad tradeoff.) One of those I wanted was useful categories -- by default you just have review comments with the only alternative being a bright red Defect comment that would block closing the review until addressed -- but it was rather annoying to all parties and so few people used it. Different conventions arose to try and address the issue but they tended to be team specific. One of my own was prefacing those low priority things with "Nitpick: " or similar like in "Nit: add space between if and (". Another convention came from being able to make file-level comments that aren't targeted at a particular line, the reviewer might preemptively check off the file if they don't expect any action and it's just broader discussion/commentary, or leave that step undone until some sort of response (maybe just clarification) or possibly broad code changes has been done.
I've sometimes had trouble convincing some older programmers about using new (or even not so new but slightly 'advanced') language features like Java lambdas or Optional or generic types (juniors/interns were often aware of and happy to use the new stuff already), to the point that for specific individuals I'd just give up and focus my review efforts on other aspects. However if I ended up touching that code myself later on, or someone else did whom I was reviewing, I'd use those newer features/encourage the other person to use them if they weren't already, and if the original person ever came back to it they'd face an argument of local consistency against trying to change it back to using the old ways. It seems like the approach of longer-term encouraging and supporting a pocket of engineers pushing for better practices would actually work with this service given the ability to make notes for future reviewers. Another flaw in Code Collab was its atrocious search which made it hard to find prior reviews about a set of files.
I noticed the example image: "It looks like this method needs to use the read lock because it's accessing the `dataKeys` map. I would recommend documenting which methods are and are not meant to be safe for concurrent access." To me, this is a problem addressed by Rust's &/&mut separation, which can be imitated in other languages (eg. in multithreaded code, wrap types in Mutex<T>/MutexGuard<T> or RwLock<T> with read guards lending out `const T&` and write guards lending out `T&`, if you're in a language without const, tough luck). This creates code which self-documents concurrency in the type system like Rust, though it's less watertight since you can hold onto references for too long.
LOL. This will become in less than 1 month a "LGTM" feast. 3rd world countries developers (organizing themselves in groups to pass the intro test from the website) giving green lights to random people around the world for peanuts. This is a clever business model if you ask me.
Let's be clear for a second. Nobody, not event the legendary 10x developers can review properly code without context or everything mentioned here: https://news.ycombinator.com/item?id=29624787
If we want our code to be approved by random people doing it only for money, this will work for sure, but it's not how it works in the real world.
Could you clarify what you mean by "LGTM?" Just people saying, "looks good to me" and doing crappy reviews, or is there some LGTM "thing" out there that everybody will use? Or something else?
There is a meme out there in the wild (or at least, I think there is a meme) that many reviewers would simply approve PRs blindly leaving a LGTM (looks good to me) comment when their internal monologue is closer to "...I have no idea what's going on here and I'm too afraid to ask at this point". I suppose it could also be sheer laziness of not wanting to parse many lines of new code, especially if you're in a situation where you don't have a 100% tight understanding of the codebase (as I have been in many a times).
PullRequest actually reviews the reviewers (I actually became a better reviewer via this).
'LGTM' is considered a bad practise, and I've been pulled up on this several times. Instead, we're trained to take more time, go deeper, and be very clear about what the changes do, and how well they are written even on relatively trivial changes.
As an outsider, I can't say anything more. I know that US/Canadian developers are in no way guarantee that LGTM feast won't happen but that's PullRequest's policy.
I'd have some amount of healthy skepticism over obvious concerns (most prominently, keeping IP safe) but it also it seems worth a shot; it solves a real problem because often for teams code reviews are a bottleneck and bit of a battlefield.
The best code reviews are ones that leverage experience from the reviewer to stop logic errors. Your code may well compile and the tests you concocted may pass but that doesn’t mean your code should ship. Without a second pair of experienced eyes looking at your crap, how are you to know that:
- (general experience with modelling) what you felt should be a single class would be better if it were factored into two classes; and
- (specific experience with this particular codebase) the second of those classes already exists in the codebase and can be found in com.clown.util?
The former can be helpful but it’s just noise compared to the value of the latter.
How does this service avoid being just a human powered style linter?
I noticed this requires LinkedIn for signup, that's too bad. I deleted my account years ago once it became a major source of spam. I think this is the first place I'm seeing it required. Strikes me as an odd requirement.
It's a bit crazy to me you'd outsource this to third party. Code reviews are the single best way spread context on a team so more than one person understands (and consents) to the change that is being made. Giving this up to 'free up engineers' for reviews seems like short term thinking. On the other hand, using this to screen hires using projects instead of inane leet code interviews seems like a good idea - especially if they worked with multiple large companies so it's many to one.
They have the Google logo in both "Work with world-class engineers" and "See why thousands of teams trust PullRequest". With the latter being next to the customer testimony section, if they are only suggesting that Google engineers do the reviewing that's quite disingenuous.
Im a 1 MAN SHOP and i hire contractors ( engineers). I am not sure if this a good fit for me? My main concern is of course cost. Do you provide feedback on what todo with the software stack? ( can my guys implement a new plan provided by you?)and 2nd question is do you provide pay for service, for example if i am interested in a new software architecture and my guys just code it up / follow your plan / recommendation. Thx
Can someone give a solid and succinct review of doing this as a side-job? What was 1) the hourly pay, 2) language(s) you reviewed, and 3) how was the work?
Sure, I'd be happy to give you some insight and background.
A little background: I've been doing software dev for 20+ years ranging from C++, Ruby (not Rails), front-end/full-stack dev. I make enough in my day job to be plenty happy.
I won't comment on pay (it's good enough for me!) as I'm not sure if it's standardized across the board, but let me just say that I was going to start moonlighting since my current role isn't as much development as I am used to....but I decided that PR was going to be a better fit and here's why:
1) I enjoy mentoring! I love interviewing candidates, doing reviews for my teammates, being a good role-model for junior devs to follow. I've taught and helped folks transition from non-tech to a tech jobs. And as a reviewer for PR, it's a lot of the same sort of thing that I already enjoy doing!
2) It helps me become a better programmer! Seeing novel or interesting solutions is always fascinating to me. I love to learn something new, and reviewing code I'm able to see the mistakes (or learn what didn't work) and then able to then take that knowledge and pass it on!
3) Much more flexibility. I can work as little, or as much as I want. I don't have a client asking where the MVP is. I don't have to worry about project planning, hitting milestones, endless meetings. I do enough of that in my day job. I can pick and choose what I want to review, when I am able. As a reviewer, there is a list of pending reviews to choose from based upon our experience and expertise in certain languages and frameworks. For instance, I often review C++, NodeJS, and React/Angular/VueJS pull requests. I tend to enjoy working on reviews for the same organization so I can pick up on the architectural design, coding style, and on-going design decisions being made...especially when I review code from a new developer joins the organization.
4) The work has been enjoyable and stress-free. I routinely do about 10-20 hours/week (in addition to my 40 hours/week day job). And it's not a fire and forget model. When I perform the review, I'm working with the developer until the review is merged. Sometimes I'll review the same piece of code multiple times as they/we work through feedback given by not only myself, but other members of the team. I am able to see comments from other PR reviewers as well as reviews from the organization.
Another aspect that I haven't seen mentioned much in the comments is that we also get to review code challenges as well! Sometimes organizations don't have the expertise or time to evaluate developers. As a reviewer, I'm able to come in and provide that feedback to the organization on how the perspective candidate did.
5) The folks at PR are also really helpful. They review the reviewers and can provide good feedback, encouragement, and suggestions in order for me to provide better reviews!
Thanks for the info! This all seems good but it basically seems like what you'd expect from gig-work code reviewing. I'm mainly concerned about the pay and how it relates to experience, haven't been able to see any comments here showing what people actually made for N hours or work. That's always the catch with the gig-work type of jobs.
I did tutoring online a while ago which claimed a lot of the same benefits, but in actuality it was hard to get a good amount of work on the site to make it worth it. You'd have to be online for hours checking the tab and then hope a 30 min session will pop up. I'm guessing there won't be that same issue here but don't want to go through the process without a general ballpark of expected income, and people being evasive in this post is kinda concerning
Putting aside IP and feasibility concerns, what is the longterm roadmap? This is obviously not a VC scalable product business if it acts as a broker to (albeit expensive) human consultants.
Are they hoping to get enough training data from the consulting practice to bootstrap an AI code review product?
This reminds of the first (and only) time I had a full 360 degree review: contradictory, zero-context opinions from people I don't know very well who haven't earned the right to interact with me in a way required for this minefield of feedback.
Except I don't believe in code reviews. Over the years I have experimented with pair programming and I think it is way better solution.
The one catch with pair programming is that some people just prefer to work alone and it is really draining to be talking constantly for couple of hours. I have modified the system to do on/off pair programming (ie. split up a little bit of work, rejoin later in the day, share what we have done, and work a little bit together).
The basic, unsolvable issue with code reviews is that the review is done after the code has already been written. As you know, the cost of fixing a problem is larger the later in the process you catch it. Pair programming aims to accomplish the correction while the code is being written.
Another huge problem is that, because the reviewer is not taking part in the development, he/she does not have the same level of understanding of what was supposed to be done.
Also, code reviewers are typically disincentivized from doing review well:
* They have other tasks to accomplish, review takes their time away from those tasks but the deadlines are not pushed automatically,
* The review tends to land at a random point in time disrupting their flow -- they have something else in mind already and they want to switch to their work as quickly as possible -- meaning they will not want to get into great detail with understanding the problem.
This causes reviews to usually be very shallow and focused on trivia. Usually, I see reviewers read the code file by file line by line, hundred times faster than it was written. It is absolutely impossible to verify a large change like that and this guarantees that they will not actually verify it thoroughly.
Yeah, you may find superficial flaws, but that's about it.
Other problems:
* The developer feels resentment because he/she thought it was all done.
* A lot of effort was spent on a wrong solution which is lost productivity.
* From project management PoV it is a problem because we can't tell how much time/effort it will take until last second.
* Reviewers feel pressure to find something so they will just report trivia if they can't find real issues.
* Reviewers tend to not want to report huge issues that would require complete rewrite because they are developers themselves and wouldn't want the same happen to them, and also because they are typically members of the same team.
I agree with your points, but the pair programming also has downsides, main one being that it doubles the total cost per task, and also it's usually limited to 2 pairs of eyes (again because of the cost), while code reviews can (and should) be done by as many members of the team as possible.
That “doubling of cost” thing is utter rubbish. If you produce higher quality code with two people deeply aware of the design and maintenance of a system doesn’t that reduce overall cost? I swear this “doubling cost” is one of those myths perpetuated by Taylorist managers.
I’ve never seen any evidence to suggest pairing is less efficient overall.
> I’ve never seen any evidence to suggest pairing is less efficient overall.
It's not less efficient, but is it really 2x as efficient as having a single senior dev per task, with proper code reviews and tests in place? And it does cost double upfront, you can't avoid that and costs are the limiting factor for many (especially smaller) teams... some of this cost will pay back in terms of bugs being less likely and easier future maintenance and all that, but can you really claim that it will absolutely ALWAYS return the initial investment? To paraphrase on your own words: "I’ve never seen any evidence to suggest it's guaranteed to be that much more efficient".
IME (and I did a lot of pair programming in my career) it depends a lot on the people being paired, how complementary their way of thinking is, and also on the particular task being worked on. It will never produce a worse code than solo programming, that's for sure, so it's great if you can afford it, but if you can't there are other ways around it to come close (code reviews being one), and that was the whole point of my comment.
You argue the pairing is more expensive and then give arguments to the contrary.
To be sure, pairing, in order to be effective, has to be properly planned and executed. It is unlikely to work if you just leave it to your employees to figure out (unless you have some really good folk that can figure it out).
> main one being that it doubles the total cost per task
Oh, that is false. I guess it comes from a simplistic understanding of how people work (no, they are not robots). You may have two people engaged in the same task, but:
* People are more focused and work faster -- it is much easier to stop yourself from procrastinating when you have other person on the call.
* You get people exchanging tacit knowledge. With people changing pairs knowledge will tend to spread over entire team eventually very efficiently.
* You get flexibility of having any of the two people being able to continue the task (if one quits, is sick, has to take day off or step out for a meeting) -- the work is much more likely to progress uninterrupted.
* You get much less chance for the project to get stuck. When one person doesn't know how to do something the other person might know.
* You will tend to get better quality results (for example any bug needs to pass through two pairs of eyes, etc.) -- and that means less time wasted on other parts of the pipeline, less technical debt, etc.
* When you get a new dev on the team, the first assignments are much less likely to get screwed up because you have other seasoned dev in the pair.
* You get a natural mechanism to get a new dev onboarded and have knowledge transfer -- they get up to speed many, many times faster than if they are just dropped alone on a task.
* Good code review isn't free either, it would have to take a significant portion of the effort to write the code anyway.
* You get people socialising while doing useful work. Which is extra difficult while working remotely. Having tightly knit team is very valuable.
* People are overall more happy and engaged when the work goes faster. Have you noticed you feel better when you stand in one long queue that progresses fast than if you split it into multiple queues that progress slowly, even if overall wait time is the same?
* Having work done faster (by two people working on it at the same time) makes for faster cycle time and in consequence lower complexity (less things being worked on at the same time). Lowering complexity is very valuable.
* While we are at complexity -- normally doubling people in the project does not cause it to progress twice as fast or cause the throughput to be twice more. Being able to treat two developers as one, uber-developer doing work more efficiently is very valuable because it allows counteracting at least part of that diminishing returns trend (ie. you have have larger team with efficiency of a smaller team).
* As a tech lead this is fantastic way for me to get to know people, their strengths and weaknesses. I can then help them with weaknesses and maybe learn something from their strengths. Getting to know the complete picture of the team is extremely valuable.
And many other reasons.
When you take that into account you will find that, if done correctly, pair programming will be more efficient. Especially long term, because some of the effects take time to kick in.
**
Now, working in pairs isn't free:
* Working in pairs requires people to have high standards when it comes to their behaviour towards their peers. Working in pairs requires absolute adherence to the "no asshole" rule.
* As a manager, you need to react very quickly to people having trouble working together because in pair programming this is going to immediately destroy productivity for two people.
* Costs of disruption is magnified when working in pairs. For example, if you have to wait for approval for something -- now you have two discontent people waiting for the approval. If one person has to join a meeting -- the other person will not be as efficient and they will have additional cost of syncing afterwards.
* You probably want to hire people that are specifically mentally designed to be able to work in pairs. Some people just prefer to isolate themselves and work alone -- these will have trouble functioning in a team that uses pair programming. It doesn't mean these people are worse, they are just worse for that particular team.
I've greatly improved as a developer by reviewing code, this improved my project and improved team alignment, that's much more valuable than getting inputs from human linter.
A service like this would only be able to find a certain class of issue. Basically syntax, but not semantics. There is no substitute for deep knowledge of a particular codebase.
A really cool side effect of working with pullrequest.com over the past year is that I do feel that I've gotten to know some of these projects. Some interactions end up becoming lengthy conversations between the reviewers and the engineering teams over the course of multiple pull requests. We point out potential issues and are still around when those issues are addressed. For some projects, the reviewers are treated like part of the team.
On top of what others have already flagged this is a big no for any companies security & compliance. Why would any company share their private codebase ?
Great question; security and compliance is a very big consideration for our customers. All code review on PullRequest is done within the platform, engineers in the PullRequest network cannot clone branches like in a garden variety source control, and we have a number of tools to give clients as much control as possible as to what our platform and engineers in our network are exposed to (e.g., https://docs.pullrequest.com/pullrequest-docs/code-review-se...).
We work very closely with our customers to ensure configurations are set up to provide our engineers with adequate context while limiting or outright restricting exposure of things they want private private.
This is also a big part of why PullRequest Reviewers are by and large restricted to US-based engineers. This ensures accuracy and consistency of criminal background checks and ease of enforceability for our non-disclosure agreements. From a legal risk assessment perspective, using PullRequest is similar to hiring a technical consultant.
I am a reviewer on pullrequest.com and I can assure you I do not work for free. Many of us are Senior or Lead developers with day jobs, who do this as a side gig and to keep our skills sharp.
During my code reviews, which tend to revolve around Angular and Ionic (that's what I signed up for), I have found lots of outdated practices. I can often provide advice on how to make their code better, show them features that are deprecated and how to address them, and generally make their code better.
As another reviewer has pointed out, we can see the entire code base, not just the current diff. We tend to work with the same companies repeatedly and become familiar their project. Some of the engineering teams treat us as part of their team and ask for advice, which is really cool.
Doing code reviews for pullrequest.com has also made me a better reviewer in my day job and has changed the way I approach my coworkers. It's truly been a win-win.
i've used pullrequest a bunch and had great experiences with the product and company.
mostly used it in small companies with 1-2 devs where it helps to get another pair of eyes on it. also, helpful as career development / learning for a junior dev.
As a PullRequest reviewer, I think many on this thread are missing the forest for the trees.
First, I think many here may be viewing Code Review as a Service from their frame of reference: from unicorn start ups, FAANG tech companies, prestigious universities, etc. Many of the companies that we help aren't coming from this world. They're small startups, looking for technical guidance. They're entrepreneurs who need to ensure they're not being duped by app developers. They're older, less tech-savvy companies that are looking to modernize. These companies need help, and they can get help from people who have technical expertise. (And by the way, some companies aren't sophisticated enough to set up linters or code scanners at their stage of development)
In a similar vein, many of the developers we help may not have had the same level of education, learning, or coaching as you or I may have. For example, I've helped introduce more modern syntax options to developers, such as string interpolation and extension methods in C#, to Options and Streams in Java, to filter/map/reduce functional patterns and optional chaining in Javascript. These developers may have never seen high quality code or had mentors who insisted on a high bar for code quality.
Even for more sophisticated customers, I've left comments ranging from security vulnerabilities (e.g. SQL injection), errors in boolean logic, recommendations for improved test coverage, recommendations for simplifying code (e.g. creating reusable functions), preventing race conditions, and more. I've also reviewed candidate assessments to help unburden senior engineers so they can focus on writing code.
Sometimes, the proof is in the pudding. There's a market for these services and that's why some companies pay for them and why I get to review code on demand. I periodically get feedback from the teams I help that I've done 'Nice Work', receiving positive ratings from the developers I review for. I'm proud that I can lend my expertise to make code better.
100% agreed. This is probably useful to enforce standards in a team that might be lacking it otherwise. For big tech teams, it might be useful to do such CRs randomly to make sure devs in the company are aligned on best practices.
I think I might be onto something here. "Full time developers as a service"