Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Code Review as a Service

pullrequest.com

Post reply on HN
Print view View original

101–110 of 238 posts

Re: Code Review as a Service

#101

Post by charcircuit »

YPCrumble wrote: post #66
alkonaut wrote: post #44

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 proje…

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.

without context you can't catch subtle mistakes

Re: Code Review as a Service

#102

Post by ivanche »

pelasaco wrote: post #20

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?

"All of our employees, contractors, and reviewers are based in the US and Canada. "

https://www.pullrequest.com/faq/

Re: Code Review as a Service

#103

Post by ivanche »

napolux wrote: post #77

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 h…

It seems they don't accept any reviewer outside US and Canada, let alone 3rd world countries.

https://www.pullrequest.com/faq/

Re: Code Review as a Service

#104

Post by 8organicbits »

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.

Re: Code Review as a Service

#105

Post by gwbas1c »

tyler_mann wrote: post #70
alkonaut wrote: post #44

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 proje…

Disclaimer, I'm the CTO @ PullRequest. 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…

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.

Re: Code Review as a Service

#106

Post by chillingeffect »

cannonpalms wrote: post #27

This entirely misses the point of code review. 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.

> 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.

Re: Code Review as a Service

#108

Post by nyanpasu64 »

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/MutexGuard or RwLock 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.

Re: Code Review as a Service

#109

Post by ivanhoe »

cannonpalms wrote: post #27

This entirely misses the point of code review. 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.

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).

Re: Code Review as a Service

#110

Post by spmurrayzzz »

alkonaut wrote: post #98
YPCrumble wrote: post #66

Earlier quoted context omitted.

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 impleme…

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.

Post reply on HN