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 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.
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.
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 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.
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.
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)
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 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.
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.
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'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.
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.
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
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"
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.
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.
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
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.
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.
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~
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.
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.
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)
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 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.
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.
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).
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."
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?
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.
> 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.
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).
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...
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
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.
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.
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.
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.
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"
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.
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.
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.
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.
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 …
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.
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.
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'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 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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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?
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'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 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.
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.
237 comments
[ 5.1 ms ] story [ 248 ms ] threadBecause 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?"
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).
"Automation" = static analysis or something like https://codeql.github.com/?
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.
I really need to find a good mentor for our project :(
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.
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?
And no, you can’t write programs to do this.
Maybe due to limitations on team size, it's not possible to have an in-house expert on every technology used.
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)
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.
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.
Perhaps that is what this service is trying to create behind the scenes: building datasets for a high-signal-to-noise-ratio automated reviewer.
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.
(In this case, they seem to be claiming Google engineers are moonlighting as reviewers, not using the service for reviewing Google’s code.)
https://techcrunch.com/2017/12/07/pullrequest-pulls-in-2-3m-...
Thanks, I hate it.
> 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.
They are operating just like a freelancing platform, right? How many hours is a "week" for them? 40 hours?
> PullRequest’s payment rates are comparable to those of a senior-level engineer based in the US.
Like include life and health insurance, paid vacation, profit-sharing, a generous signing bonus, and more?
It's pretty clear that the people offering the service are doing it as a way of gaining extra income on the side rather than a main employment.
$50-3000 / week tells you nothing.
They should tell you the avg $ per hour.
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
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"
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.
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.
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
https://www.pullrequest.com/faq/
Say it with me: Code review is a knowledge transfer exercise.
Finding bugs, security vulnerabilities, and keeping the code maintainable are merely side effects that we appreciate along the way.
The primary purpose of code review is increasing the bus factor of the given piece of code and facilitating organic knowledge transfer. That's it.
This, of course, horrified me.
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)
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.
https://www.careerexplorer.com/careers/solution-architect/
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 ;-)
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.
"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.
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.
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).
It must be "nice" working with an enterprise architect who views agile as a strict, narrowly defined thing.
https://www.joelonsoftware.com/2000/08/09/the-joel-test-12-s...
Still sounds like a pretty valuable service.
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?
Simultaneously, architecture is complete mess, because architecture is much harder to see in code review.
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.
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).
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?"
In many places, yes.
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...
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.
I'll leave a third opinion from here https://static1.smartbear.co/smartbear/media/pdfs/best-kept-... which lists some direct and indirect benefits:
Code review for KT is one thing. Code review for finding bugs is another. Code review for following style guidelines is yet another.
I would like to use a baseline style guideline for JS is anyone aware of one that isn't too huge?
Prettier is popular for that job:
https://prettier.io/
For detecting functional/idiomatic/behavioral issues, ESLint is my go-to:
https://eslint.org/
This shows my bias for automation over human enforcement.
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.
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.
I think I might be onto something here. "Full time developers as a service"
Edit: to be clear, I personally don't see the value here, just playing devil's advocate.
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.
It's Rice's Theorem which tends to get me in trouble!
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 …
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.
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?"
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.
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.
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.
If it's written "very well" then what's the point of a pull request code review? /headscratch
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.
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.
Sounds like you need two or three people dedicated to the task. Documentation is a whole profession by itself. Well, good documentation.
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.
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?
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.
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 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.
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.
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...
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.
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.
Or are these people working for relative poverty wages overseas?
No need to guess, it takes 20 seconds to check: https://app.pullrequest.com/signups/reviewer
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.
Are they hoping to get enough training data from the consulting practice to bootstrap an AI code review product?
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.
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.
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.
This is to me a set of linters with human error involved..