[HN Gopher] Code Review as a Service
___________________________________________________________________
Code Review as a Service
Author : dennisy
Score : 261 points
Date : 2021-12-20 11:09 UTC (11 hours ago)
(HTM) web link (www.pullrequest.com)
(TXT) w3m dump (www.pullrequest.com)
| gandutraveler wrote:
| 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 ?
| ddm379 wrote:
| Disclaimer: I'm Dan Mateer, COO at PullRequest
|
| 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.
| nathanielarmer wrote:
| I'm a reviewer on PullRequest, and thought I'd share some
| perspective. Happy to answer any questions in comments.
|
| Background - I've built and led engineering teams at multiple
| fast-growing startups. In doing so I've seen the incredible value
| PR's can provide, but also the huge cost of them on small teams.
| I review on PullRequest part-time as I work full-time building a
| startup.
|
| Many of the critics here are right. The PullRequest service won't
| catch every bug. As reviewers on this service, we lack context*
| to fully understand the impact of every code change.
|
| However, this can be a blessing in disguise. As an outsider, I
| bring an entirely different set of context to the project. I can
| see errors or improvements that teams have become blind to, I
| don't have the pressure of shipping for X release, and I've often
| been not only where these teams are, but where these teams want
| to be in X months.
|
| This is all done without using any man-hours on the client team -
| which is often a critically short resource.
|
| Ultimately the proof is in the pudding, I raise comments on
| almost every single review I do, raising from best practice to
| architectural to security vulnerability, and the majority of the
| time teams take that feedback onboard.
|
| Other QA:
|
| Where are you located? I'm located in San Francisco.
|
| Who approves PR's? I tend to consider my role to advise and
| sometimes mentor. I will give opinions, but it ultimately the
| client's job to approve/reject PR's.
|
| My code is great, I don't need this! Have you tried it? As an
| outsider, I catch peoples blind-spots. That said, PR is always
| looking for great reviewers to join the team!
|
| * It's worth noting, that between seeing the code, optionally
| having access to the full code base, and asking questions of
| developers - I develop a decent mental model of most projects.
| Jach wrote:
| Here's a question: do you get pushback about "best practices"?
| (Sometimes really just "standard practices".) If so, which ones
| in particular? And do you actually continue trying to explain
| why it's a good idea, or just throw up your hands with a
| feeling of "I gave my advice, they can ignore it at their
| peril".
|
| Having actually convinced people of the value of certain
| practices (like favoring fast focused unit tests over slow
| broad integration tests that happen to require the full system
| to be running), which leads to smoother and more succinct
| reviews in the future -- like "please add tests" doesn't even
| need to be said if the developer values them and already wrote
| some -- I don't know if I'd put up with the stress of having to
| fight the same battles week after week with a new group. Though
| I suppose it's possible that you might get good at convincing
| people of a certain thing, and the ease of convincing can
| reveal whether the thing is really closer to "best" or just
| "standard".
| nathanielarmer wrote:
| I've gotten pushback on best practises, especially from more
| junior developers or developers on a tight deadline.
|
| I do C# and one common, yet trivial example is poor naming
| conventions.
|
| As a reviewer we have certain tools we can use to encourage
| change: * We can make comments low priority, so the advice is
| there, but it's skippable. * We can make summary comments,
| that are opinionated but don't expect any immediate or direct
| resolution (great for architectural thoughts). * We can
| include example code snippets in our comments. This reduces
| the burden for the developer to adopt a certain change. I've
| even gone to the extent of writing small programs to validate
| a refactor or prove an error exists. * Sometimes it's not an
| error, but how that company wants X done. We take note of
| these for future reviewers.
|
| Overall, this is a diplomacy game, the only power we have is
| soft power. I find it good practise, as I'm typically a
| direct kinda guy.
|
| Results?
|
| I've seen a lot of developers, after being introduced to new
| ways of working, implement that on later PR's. Sometimes
| immediately, sometimes after it arises a few times. Examples
| include improved naming, better code structure, usage of
| newer language features, more clarity in the code, or better
| SQL injection protection!
| Jach wrote:
| Thanks for the reply! It's nice to hear that a lot of devs
| are receptive to change and aren't treating the service
| hostilely. I like that there's categorization with
| expectations encoded in the category, I can see it helping
| a lot of things. A huge chunk of my reviews used to be in
| Code Collab, which... lacked certain desirable things. (But
| it got right a few others, so using it wasn't a totally bad
| tradeoff.) One of those I wanted was useful categories --
| by default you just have review comments with the only
| alternative being a bright red Defect comment that would
| block closing the review until addressed -- but it was
| rather annoying to all parties and so few people used it.
| Different conventions arose to try and address the issue
| but they tended to be team specific. One of my own was
| prefacing those low priority things with "Nitpick: " or
| similar like in "Nit: add space between if and (". Another
| convention came from being able to make file-level comments
| that aren't targeted at a particular line, the reviewer
| might preemptively check off the file if they don't expect
| any action and it's just broader discussion/commentary, or
| leave that step undone until some sort of response (maybe
| just clarification) or possibly broad code changes has been
| done.
|
| I've sometimes had trouble convincing some older
| programmers about using new (or even not so new but
| slightly 'advanced') language features like Java lambdas or
| Optional or generic types (juniors/interns were often aware
| of and happy to use the new stuff already), to the point
| that for specific individuals I'd just give up and focus my
| review efforts on other aspects. However if I ended up
| touching that code myself later on, or someone else did
| whom I was reviewing, I'd use those newer
| features/encourage the other person to use them if they
| weren't already, and if the original person ever came back
| to it they'd face an argument of local consistency against
| trying to change it back to using the old ways. It seems
| like the approach of longer-term encouraging and supporting
| a pocket of engineers pushing for better practices would
| actually work with this service given the ability to make
| notes for future reviewers. Another flaw in Code Collab was
| its atrocious search which made it hard to find prior
| reviews about a set of files.
| ssully wrote:
| I see a lot of comments regarding lacking context/knowledge of
| the full system/integration. I get that, but I think there is
| real value here. I have absolutely worked on teams where they
| were fire fighting for months at a time, where there were 1-2
| developers total, and just not enough time or resources to do
| proper code reviews. While this service could miss big picture
| things, it could absolutely catch low-mid tier issues.
|
| My biggest issue would be from a security perspective. The
| background checks and everything are nice, but there are some
| systems I would just never let this service touch.
|
| Overall, this is interesting! Wish the team behind it the best of
| luck.
| imglorp wrote:
| I hope my boss doesn't think, "It's cheaper to pay for this
| service than for us to review our own PRs. Outsource ++!"
| GrahamCampbell wrote:
| Cheaper in the long term to do both. :)
| est wrote:
| I hope it can review popular code on github.com automatically.
| [deleted]
| jcadam wrote:
| I received an invite to pullrequest nearly a year ago, filled out
| the application, and never heard from them again.
| grenran wrote:
| There's no way that Google would actually use this service.
| ceejayoz wrote:
| SaaS apps tend to slap a logo up when there's a single free
| trial account with a @google.com email address.
|
| (In this case, they seem to be claiming Google engineers are
| moonlighting as reviewers, not using the service for reviewing
| Google's code.)
| adamnemecek wrote:
| Google has invested in pullrequest and some engineers review
| for pull request
|
| https://techcrunch.com/2017/12/07/pullrequest-pulls-
| in-2-3m-...
| pledg wrote:
| 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.
| walterbell wrote:
| _> expert engineers backed by best-in-class automation._
|
| "Automation" = static analysis or something like
| https://codeql.github.com/?
| GrahamCampbell wrote:
| I have performed reviews on pullrequest.com, and lack of context
| tends not to be an issue. Code review is an interactive process,
| where questions can be posed to the PR authors, and PR reviewers
| have access to search the codebase. Customer success at
| pullrequest.com also provide context for the repo and
| organization working practices, attached at the top of PR
| descriptions, to help reviewers with context and to know what
| kinds of feedback are useful. Sast and Dast are still not
| sophisticated enough or widely used enough to be good enough. In-
| house senior and principal engineers often do not have the time
| to be on top of every repo and every PR in their organization,
| and need to outsource an extra pair of eyes. It is highly common
| that I am reviewing codebases that do not have any Sast or Dast,
| or have workarounds to silence the tools, resulting in bugs and
| security issues slipping through. It is very rare that I am able
| to approve PRs without comments, even after an internal review
| has taken place!
| dm03514 wrote:
| > Code review is an interactive process, where questions can be
| posed to the PR authors, and PR reviewers have access to search
| the codebase.
|
| I also performed reviews on pullrequest.com (~2+ years ago?). I
| firmly believe code reviews are an interactive and knowledge
| sharing process but was explicitly told NOT to ask questions
| during pull requests, but instead to make statements focusing
| on bugs and style.
|
| Based on the comments here, this def looks valuable to some of
| pullrequests.com customers. I believe this is more of a human
| acting as a safety net, or risk mitigation strategy, and loses
| the majority of the value that pull requests can provide to an
| organization.
| spmurrayzzz wrote:
| Just curious about the flavor of code you were reviewing. Can
| you elaborate a bit on that? Specifically - any software
| integrations with hardware? E.g. maybe a distributed cloud
| service that talks to on-prem firmware or similar (think IoT,
| cisco/juniper network gear, etc).
|
| There is a whole class of software that I can't imagine someone
| could come in blind to and offer any real value to necessitate
| the balance sheet exposure. But I'm very interested to hear if
| folks there have had to do this (yourself or others).
| skeeter2020 wrote:
| This reminds of the first (and only) time I had a full 360 degree
| review: contradictory, zero-context opinions from people I don't
| know very well who haven't earned the right to interact with me
| in a way required for this minefield of feedback.
| andrewmwatson wrote:
| I've been a reviewer on PR for a couple of months now and it's
| been amazing. I've gotten to help dozens, maybe hundreds of
| developers improved their code, fix security, readability,
| testing and lots of other issues. Sometimes the PR is short and
| the review is short but sometimes it's much more involved and
| I've had the opportunity to really explain things and educate
| people in a way that no lint checker could. I'm able to apply the
| wealth of my experience and knowledge with specific guidance
| based on things I've learned from building software for 22 years.
| I can't think of another way that a company could gain access to
| that kind of guidance for only $699 a month.
|
| I can go into detail explaining how to safely and reliably
| structure something and the feedback I get from the developers is
| positive and appreciative. I also collaborate with other
| reviewers about things we're seeing and I've learned a ton in the
| process because the other reviewers they've all got such a depth
| of experience and knowledge in different areas.
|
| I don't do reviews full time, I have a day job, but I've been
| earning about $1000 a week doing it just some nights and weekends
| and that has been really great too.
|
| Overall, I think PullRequest is an enormously positive thing for
| its customers and the reviewers. We're not trying to replace
| people's existing review processes if they have them. Our goal is
| just to add to our customers' capabilities and I think we're
| going very well at that.
| janikvonrotz wrote:
| Marked as spam.
| lmilcin wrote:
| Except I don't believe in code reviews. Over the years I have
| experimented with pair programming and I think it is way better
| solution.
|
| The one catch with pair programming is that some people just
| prefer to work alone and it is really draining to be talking
| constantly for couple of hours. I have modified the system to do
| on/off pair programming (ie. split up a little bit of work,
| rejoin later in the day, share what we have done, and work a
| little bit together).
|
| The basic, unsolvable issue with code reviews is that the review
| is done _after_ the code has already been written. As you know,
| the cost of fixing a problem is larger the later in the process
| you catch it. Pair programming aims to accomplish the correction
| while the code is being written.
|
| Another huge problem is that, because the reviewer is not taking
| part in the development, he/she does not have the same level of
| understanding of what was supposed to be done.
|
| Also, code reviewers are typically disincentivized from doing
| review well:
|
| * They have other tasks to accomplish, review takes their time
| away from those tasks but the deadlines are not pushed
| automatically,
|
| * The review tends to land at a random point in time disrupting
| their flow -- they have something else in mind already and they
| want to switch to their work as quickly as possible -- meaning
| they will not want to get into great detail with understanding
| the problem.
|
| This causes reviews to usually be very shallow and focused on
| trivia. Usually, I see reviewers read the code file by file line
| by line, hundred times faster than it was written. It is
| absolutely impossible to verify a large change like that and this
| guarantees that they will not actually verify it thoroughly.
|
| Yeah, you may find superficial flaws, but that's about it.
|
| Other problems:
|
| * The developer feels resentment because he/she thought it was
| all done.
|
| * A lot of effort was spent on a wrong solution which is lost
| productivity.
|
| * From project management PoV it is a problem because we can't
| tell how much time/effort it will take until last second.
|
| * Reviewers feel pressure to find _something_ so they will just
| report trivia if they can 't find real issues.
|
| * Reviewers tend to not want to report huge issues that would
| require complete rewrite because they are developers themselves
| and wouldn't want the same happen to them, and also because they
| are typically members of the same team.
|
| * and so on.
| ivanhoe wrote:
| I agree with your points, but the pair programming also has
| downsides, main one being that it doubles the total cost per
| task, and also it's usually limited to 2 pairs of eyes (again
| because of the cost), while code reviews can (and should) be
| done by as many members of the team as possible.
| tcbasche wrote:
| That "doubling of cost" thing is utter rubbish. If you
| produce higher quality code with two people deeply aware of
| the design and maintenance of a system doesn't that reduce
| overall cost? I swear this "doubling cost" is one of those
| myths perpetuated by Taylorist managers. I've never seen any
| evidence to suggest pairing is less efficient overall.
| lmilcin wrote:
| > main one being that it doubles the total cost per task
|
| Oh, that is false. I guess it comes from a simplistic
| understanding of how people work (no, they are not robots).
| You may have two people engaged in the same task, but:
|
| * People are more focused and work faster -- it is much
| easier to stop yourself from procrastinating when you have
| other person on the call.
|
| * You get people exchanging tacit knowledge. With people
| changing pairs knowledge will tend to spread over entire team
| eventually very efficiently.
|
| * You get flexibility of having any of the two people being
| able to continue the task (if one quits, is sick, has to take
| day off or step out for a meeting) -- the work is much more
| likely to progress uninterrupted.
|
| * You get much less chance for the project to get stuck. When
| one person doesn't know how to do something the other person
| might know.
|
| * You will tend to get better quality results (for example
| any bug needs to pass through two pairs of eyes, etc.) -- and
| that means less time wasted on other parts of the pipeline,
| less technical debt, etc.
|
| * When you get a new dev on the team, the first assignments
| are much less likely to get screwed up because you have other
| seasoned dev in the pair.
|
| * You get a natural mechanism to get a new dev onboarded and
| have knowledge transfer -- they get up to speed many, many
| times faster than if they are just dropped alone on a task.
|
| * Good code review isn't free either, it would have to take a
| significant portion of the effort to write the code anyway.
|
| * You get people socialising _while_ doing useful work. Which
| is extra difficult while working remotely. Having tightly
| knit team is very valuable.
|
| * People are overall more happy and engaged when the work
| goes faster. Have you noticed you feel better when you stand
| in one long queue that progresses fast than if you split it
| into multiple queues that progress slowly, even if overall
| wait time is the same?
|
| * Having work done faster (by two people working on it at the
| same time) makes for faster cycle time and in consequence
| lower complexity (less things being worked on at the same
| time). Lowering complexity is very valuable.
|
| * While we are at complexity -- normally doubling people in
| the project does not cause it to progress twice as fast or
| cause the throughput to be twice more. Being able to treat
| two developers as one, uber-developer doing work more
| efficiently is very valuable because it allows counteracting
| at least part of that diminishing returns trend (ie. you have
| have larger team with efficiency of a smaller team).
|
| * As a tech lead this is fantastic way for me to get to know
| people, their strengths and weaknesses. I can then help them
| with weaknesses and maybe learn something from their
| strengths. Getting to know the complete picture of the team
| is extremely valuable.
|
| And many other reasons.
|
| When you take that into account you will find that, if done
| correctly, pair programming will be more efficient.
| Especially long term, because some of the effects take time
| to kick in.
|
| **
|
| Now, working in pairs isn't free:
|
| * Working in pairs requires people to have high standards
| when it comes to their behaviour towards their peers. Working
| in pairs requires absolute adherence to the "no asshole"
| rule.
|
| * As a manager, you need to react very quickly to people
| having trouble working together because in pair programming
| this is going to immediately destroy productivity for two
| people.
|
| * Costs of disruption is magnified when working in pairs. For
| example, if you have to wait for approval for something --
| now you have two discontent people waiting for the approval.
| If one person has to join a meeting -- the other person will
| not be as efficient and they will have additional cost of
| syncing afterwards.
|
| * You probably want to hire people that are specifically
| mentally designed to be able to work in pairs. Some people
| just prefer to isolate themselves and work alone -- these
| will have trouble functioning in a team that uses pair
| programming. It doesn't mean these people are worse, they are
| just worse for that particular team.
| rpunkfu wrote:
| I wish person (people) involved in making this would do some more
| research, it's completely missing a point of PR review.
|
| This is to me a set of linters with human error involved..
| aneutron wrote:
| 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.
| dennisy wrote:
| My thought is the same - I was wondering if I had missed
| something!
|
| I really need to find a good mentor for our project :(
| whazor wrote:
| 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.
| noduerme wrote:
| 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.
| cheriot wrote:
| This smells like lead gen for a consulting shop.
| whoomp12342 wrote:
| very often code review requires domain knowledge. This seems like
| it would devolve into arguing over coding preferences
| natded wrote:
| Teaching sand to think was a mistake.
| cannonpalms wrote:
| 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.
| vinceguidry wrote:
| 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.
|
| This, of course, horrified me.
| haggy102 wrote:
| 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.
| akvadrako wrote:
| Even people designing buildings?
| vinceguidry wrote:
| 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)
| dangus wrote:
| 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.
|
| 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 ;-)
| alistairSH wrote:
| 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.
| vinceguidry wrote:
| 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.
| Pxtl wrote:
| 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.
| vinceguidry wrote:
| 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.
| alistairSH wrote:
| 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).
| skeeter2020 wrote:
| >> it breaks Agile
|
| It must be "nice" working with an enterprise architect who
| views agile as a strict, narrowly defined thing.
| vinceguidry wrote:
| He's really sharp and has keen insight into how enterprise
| business works. His advice saves us a lot of time and
| money.
| throwoutway wrote:
| But if he wants you to open tech debt tickets, rather
| than spotting/fixing security issues immediately, this
| will not save the company time or money.
| vinceguidry wrote:
| He doesn't. He was just describing to me the way the
| enterprise usually sees things. Our shop is more modern
| than he's used to working in.
| ivanhoe wrote:
| 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...
| vinceguidry wrote:
| If they get solved at all.
| Pxtl wrote:
| 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?"
| Jach wrote:
| 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.
|
| I'll leave a third opinion from here
| https://static1.smartbear.co/smartbear/media/pdfs/best-kept-...
| which lists some _direct_ and _indirect_ benefits:
| 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
| sbassi wrote:
| 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.
| senko wrote:
| So, they find bugs and security vulnerabilities, help spot
| performance problems and keep the code maintainable -- as a
| service.
|
| Still sounds like a pretty valuable service.
| chillingeffect wrote:
| > 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.
| gwbas1c wrote:
| > Code review is a knowledge transfer exercise
|
| 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...
| dpweb wrote:
| Not exactly. imo and just a perspective, but code review is one
| method of KT. Sending someone a document can also be a method
| of KT.
|
| 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?
| NateEag wrote:
| I'm a fan of using code formatters to define how you lay out
| code in a file.
|
| 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.
| ivanhoe wrote:
| 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).
| walkingriver wrote:
| 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.
| pictur wrote:
| code review is a process that carries a lot of meaning today.
| the work cycle in most workplaces is not qualified to meet this
| meaning.
| SomeHacker44 wrote:
| 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.
| astrobe_ wrote:
| 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.
| pelasaco wrote:
| "but they have AI".. probably not. Probably the same linters
| and scanners that we all know + cheap working force reviewing
| code manually.
| oneepic wrote:
| 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.
| mfashby wrote:
| Hmm, what about education? I've learned a bunch through
| thoughtful reviews of my code by other devs.
| loloquwowndueo wrote:
| That's them transferring their knowledge to you :)
| tantalor wrote:
| You're thinking of pair programming. That's different.
| nerdjon wrote:
| 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?
| watwut wrote:
| We do have detailed code reviews. People will complain about
| lines you write and expect a lot of consistency.
|
| Simultaneously, architecture is complete mess, because
| architecture is much harder to see in code review.
| beanjuiceII wrote:
| 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~
| nyanpasu64 wrote:
| I noticed the example image: "It looks like this method needs to
| use the read lock because it's accessing the `dataKeys` map. I
| would recommend documenting which methods are and are not meant
| to be safe for concurrent access." To me, this is a problem
| addressed by Rust's &/&mut separation, which can be imitated in
| other languages (eg. in multithreaded code, wrap types in
| Mutex<T>/MutexGuard<T> or RwLock<T> with read guards lending out
| `const T&` and write guards lending out `T&`, if you're in a
| language without const, tough luck). This creates code which
| self-documents concurrency in the type system like Rust, though
| it's less watertight since you can hold onto references for too
| long.
| nkmnz wrote:
| I'm currently the only developer in my startup and I crave for
| high quality feedback, so this service sounds like it's a gift
| sent from heaven - especially for the $699 price tag per month. I
| guess they probably make a mixed calculation: take a small
| company with 10 devs. Two of them are part-time, on any given
| week one of them is on holidays/sick leave, one is very junior
| and doesn't contribute much yet, another one is more concerned
| with project management than with writing code and one has in
| fact transitioned into management but remains on the dev team
| because it's part of their identity to still "get their hands
| dirty" (even though they don't have the time to produce much
| code) - so you might end up with sth like 4-7x more code than I
| write, but 10x the payments. My productivity is not above
| average, but I have very little overhead besides coding.
| Unfortunately, I cannot find any information about a minimum team
| size for the PRO plan. I think I'll give them a try.
| softwaredoug wrote:
| 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.
| vemv wrote:
| 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.
| dirtbag__dad wrote:
| I used this service at my previous employer to launch a small
| django/nextjs app and it was mostly fantastic.
|
| Background is that I worked at an VC-backed startup as a dev
| after doing General Assembly's full stack bootcamp. Left that job
| to do ops/growth, and ~2.5 years later volunteered to build the
| web app when my company put the project on their roadmap.
|
| As the only developer at the company, pullrequest was great for:
| - a general gut check on how I was doing - recommendations on how
| to better write js/python. linters help but nice to have a person
| offer feedback on more advanced ways of doing things - sourcing
| documentation on best practices. I found a lot of
| typescript/JavaScript resources to be inconsistent/confusing. Was
| great for someone to find and vet guides for me. - help with
| bugs/errors - basic library choices and architecture decisions
|
| It was also fantastic to have several people reviewing my code at
| once. Gave me a perspective on the type of engineering manager
| I'd want to work for. Some folks focused more on technical
| details but struggled explain their changes in plain English,
| while others seemed to be the other way around.
|
| pullrequest was not great for: - doing things fast. They didn't
| have a real-time messaging feature so I'd get hung up on waiting
| for feedback. They do have a 24(?) hour turnaround, but when
| several folks are commenting on the same PR it gets hard to track
| what changes _really_ matter vs what they are throwing out there
| as a nice to have. - Anything that required context outside of
| the files committed. Though with some extra long PR comments I
| could manage.
|
| I would 100% recommend pullrequest for small teams who are heavy
| on more junior devs or migrating to a new stack. It is an
| inexpensive way to ramp up learning.
|
| Last thing here -- when I worked at the startup my code was
| rarely reviewed, and I'd have to actively ask for it. Not all
| companies follow best practices (even if they have the resources
| to). I would've loved this at my past job, too.
| alexashka wrote:
| What we need is software architecture review, not git diff
| review.
| akhil-ghatiki wrote:
| How does this play with the Non disclosure agreements ?
| akhil-ghatiki wrote:
| ignore this comment. They have a background check and NDA
| process.
| swalsh wrote:
| 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.
| scubbo wrote:
| 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.
| Kocrachon wrote:
| I understand NDAs are a thing, but I still don't think I'm too
| comfortable with the idea of letting a bunch of third-party
| people look at a bunch of my internal information. And I
| understand this is targeting startups a lot more than large
| established companies. But to me that's even more concerning
| because as a startup you're really trying to move fast and hoping
| someone doesn't beat you to the punch, and you're handing a bunch
| of people you don't know how much of your secret internal
| information, and hope that they don't go talking to other people
| about it.
| vincentmarle wrote:
| Most startups are too busy finding product market fit than
| trying to worry about sharing 'secret information' (your actual
| secrets shouldn't be in your repo anyways).
| niros_valtos wrote:
| 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.
| rwmj wrote:
| 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.
| niros_valtos wrote:
| 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.
| jopsen wrote:
| 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?
| gorgoiler wrote:
| The best code reviews are ones that leverage experience from the
| reviewer to stop logic errors. Your code may well compile and the
| tests you concocted may pass but that doesn't mean your code
| should ship. Without a second pair of experienced eyes looking at
| your crap, how are you to know that:
|
| - _(general experience with modelling)_ what you felt should be a
| single class would be better if it were factored into two
| classes; and
|
| - _(specific experience with this particular codebase)_ the
| second of those classes already exists in the codebase and can be
| found in com.clown.util?
|
| The former can be helpful but it's just noise compared to the
| value of the latter.
|
| How does this service avoid being just a human powered style
| linter?
| gitdiff wrote:
| As a PullRequest reviewer, I think many on this thread are
| missing the forest for the trees.
|
| First, I think many here may be viewing Code Review as a Service
| from their frame of reference: from unicorn start ups, FAANG tech
| companies, prestigious universities, etc. Many of the companies
| that we help aren't coming from this world. They're small
| startups, looking for technical guidance. They're entrepreneurs
| who need to ensure they're not being duped by app developers.
| They're older, less tech-savvy companies that are looking to
| modernize. These companies need help, and they can get help from
| people who have technical expertise. (And by the way, some
| companies aren't sophisticated enough to set up linters or code
| scanners at their stage of development)
|
| In a similar vein, many of the developers we help may not have
| had the same level of education, learning, or coaching as you or
| I may have. For example, I've helped introduce more modern syntax
| options to developers, such as string interpolation and extension
| methods in C#, to Options and Streams in Java, to
| filter/map/reduce functional patterns and optional chaining in
| Javascript. These developers may have never seen high quality
| code or had mentors who insisted on a high bar for code quality.
|
| Even for more sophisticated customers, I've left comments ranging
| from security vulnerabilities (e.g. SQL injection), errors in
| boolean logic, recommendations for improved test coverage,
| recommendations for simplifying code (e.g. creating reusable
| functions), preventing race conditions, and more. I've also
| reviewed candidate assessments to help unburden senior engineers
| so they can focus on writing code.
|
| Sometimes, the proof is in the pudding. There's a market for
| these services and that's why some companies pay for them and why
| I get to review code on demand. I periodically get feedback from
| the teams I help that I've done 'Nice Work', receiving positive
| ratings from the developers I review for. I'm proud that I can
| lend my expertise to make code better.
| loloquwowndueo wrote:
| "LGTM" as a service :)
| hankchinaski wrote:
| 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
| gitfan86 wrote:
| 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.
| altdataseller wrote:
| Vitamin, not painkiller. Just keep that in mind
| tialaramex wrote:
| 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.
| adamnemecek wrote:
| 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.
|
| And no, you can't write programs to do this.
| tialaramex wrote:
| 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)
| noduerme wrote:
| 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?
| danuker wrote:
| > via static analysis and custom ci/cd workflow checks
|
| Perhaps that is what this service is trying to create behind
| the scenes: building datasets for a high-signal-to-noise-ratio
| automated reviewer.
| biztos wrote:
| 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.
| seneca wrote:
| This post has a number of what look like astroturf advertising
| comments.
| darkstar999 wrote:
| That's the word I was looking for. It really does look like
| that.
| dawnerd wrote:
| I had to flag this after seeing several obvious spam replies
| trying to boost the service. Come on now.
| darkstar999 wrote:
| The many multi-paragraph praises from green accounts did it for
| me.
| Rhodee wrote:
| :wave: Pull Request reviewer here with over 1000 reviews.
|
| I've reviewed code across languages, team size and maturity. A
| good static analysis tool is not code review. The word 'context'
| came up 37 times in this thread (by the time I hit submit) and it
| is worth digging in to how I build and maintain context with
| teams. First up, it is my responsibility to uphold, not define a
| team's best current practices. If you believe a linter and static
| analysis tools are best current practices, you probably do not
| need code review. The code review process is an exercise to
| affirm how well a pull request meets the expectations (context)
| of a team and suggest remediation where appropriate.
|
| I assume 'context' used in the threads to mean "how we do things
| here". As a reviewer, I conduct review understanding the problems
| the team wants its code reviews to optimize for or to avoid. This
| is due to the people creating the platform. It's a solid platform
| for reviewers to get things done. Every line of contributed code
| I review is done with an eye towards ensuring it affirms a team's
| stated objectives. If those objectives somehow falls outside of
| what I know to be true from my experiences as a professional and
| best current practices (BCP), I am empowered to engage the team.
|
| I've had teams request to never provide guidance for coding style
| issues. Others want to know if how they modeled a React component
| tree could be improved. My success on the platform relies on
| always building context. Without that rapport it limits the depth
| of the review. Because code reviews are interactive, they tend to
| get better over time. When things go well, the relationship
| between team and reviewer is seeking a pareto optimum between the
| pull request and the team's best current practices. When needs
| change I adapt my reviews. It is the same treatment if you're a
| one person shop, SME or a listed company.
|
| k thx bye
| deeveloper wrote:
| I'm a reviewer and a firm believer in PullRequest. I do believe
| that some of the concerns expressed here are valid but I think
| they belie a fundamental misunderstanding of "what you're
| getting" from PR. Having an under-experienced contractor review
| your code without context, pointing out things that static
| analysis could/would catch sounds like a terrible proposition,
| but that is NOT what PR provides.
|
| Speaking from personal experience, the reviewers are very
| knowledgable professionals who are not doing this for money as
| the primary motivation. The pay is great but is more of a
| perk/reward for doing something we enjoy, and doing a great job
| at it. The staff at PR does an amazing job of coaching and
| guiding reviewers on how to provide the most value to the client
| and also encouraging us to do our best work. It is never anything
| like "You need to be faster and bill less, do more, etc" it is
| quite the opposite, we are encouraged to "keep the clock going"
| while we research, gain context, etc.
|
| Everything is focused on providing value to the client, and IMHO
| a stellar job is done. Every review comes with detailed
| information on who the client is, the make up of their team
| (seniority, etc), recommendations on what to look for (and what
| NOT to look for) etc. Essentially you are putting your code in
| front of people who have been there, done that for a long time
| and are acutely aware of not only risk but also pain points and
| how to avoid tech debt. We are providing insight, recommendations
| and even code snippets on how to avoid repetition, speed things
| up, or make them easier to maintain.
| privacyonsec wrote:
| Isn't paper peer reviewing broken ? Want to do the same thing
| with software?
| jph wrote:
| 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.
| throwaway984393 wrote:
| 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.
| schnika wrote:
| I wanted to sign up as a reviewer but it seems like one needs a
| linkedin account to do so. Since I don't have a linkedin account
| it seems like there is no other chance?
| Cakez0r wrote:
| Me too. Hopefully someone working for the company sees this and
| has some sway to remove the requirement.
| lambic wrote:
| The only time I can see this being useful is on solo developer
| projects, and solo developers probably can't afford it.
| ralusek wrote:
| 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.
| walkingriver wrote:
| 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.
| napolux wrote:
| LOL. This will become in less than 1 month a "LGTM" feast. 3rd
| world countries developers (organizing themselves in groups to
| pass the intro test from the website) giving green lights to
| random people around the world for peanuts. This is a clever
| business model if you ask me.
|
| Let's be clear for a second. Nobody, not event the legendary 10x
| developers can review properly code without context or everything
| mentioned here: https://news.ycombinator.com/item?id=29624787
|
| If we want our code to be approved by random people doing it only
| for money, this will work for sure, but it's not how it works in
| the real world.
| dhimes wrote:
| Could you clarify what you mean by "LGTM?" Just people saying,
| "looks good to me" and doing crappy reviews, or is there some
| LGTM "thing" out there that everybody will use? Or something
| else?
| MajorBee wrote:
| There is a meme out there in the wild (or at least, I think
| there is a meme) that many reviewers would simply approve PRs
| blindly leaving a LGTM (looks good to me) comment when their
| internal monologue is closer to "...I have no idea what's
| going on here and I'm too afraid to ask at this point". I
| suppose it could also be sheer laziness of not wanting to
| parse many lines of new code, especially if you're in a
| situation where you don't have a 100% tight understanding of
| the codebase (as I have been in many a times).
| ivanche wrote:
| It seems they don't accept any reviewer outside US and Canada,
| let alone 3rd world countries.
|
| https://www.pullrequest.com/faq/
| nathanielarmer wrote:
| PullRequest actually reviews the reviewers (I actually became a
| better reviewer via this).
|
| 'LGTM' is considered a bad practise, and I've been pulled up on
| this several times. Instead, we're trained to take more time,
| go deeper, and be very clear about what the changes do, and how
| well they are written even on relatively trivial changes.
| dennisy wrote:
| Interested to know if anyone has used this service or similar and
| what the quality is like.
| codingdave wrote:
| 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?"
| Leherenn wrote:
| 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).
| alkonaut wrote:
| 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"
| andreyk wrote:
| 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.
| jermaustin1 wrote:
| But almost any good SA can catch all of those issues, a
| person doesn't need to look through the code for that.
| senko wrote:
| This is very close to saying Halting problem is not, in
| fact, a problem: https://brilliant.org/wiki/halting-
| problem/
| jermaustin1 wrote:
| 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.
| reuben364 wrote:
| A human being doesn't solve the halting problem either.
| adgjlsfhk1 wrote:
| 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.
| samatman wrote:
| Ironically, it very seldom is.
|
| It's Rice's Theorem which tends to get me in trouble!
| HPsquared wrote:
| 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.
| malux85 wrote:
| 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 ...
| alkonaut wrote:
| 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.
| YPCrumble wrote:
| 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.
| sbassi wrote:
| that is impossible in lot of cases
| fourseventy wrote:
| Not very realistic for the real world
| blacklion wrote:
| 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.
| coryrc wrote:
| 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).
| mbesto wrote:
| > it probably isn't written very well.
|
| If it's written "very well" then what's the point of a pull
| request code review? /headscratch
| alkonaut wrote:
| 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?"
| spmurrayzzz wrote:
| 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.
| NateEag wrote:
| 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.
| alkonaut wrote:
| > 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.
| charcircuit wrote:
| without context you can't catch subtle mistakes
| tyler_mann wrote:
| 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 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.
| gwbas1c wrote:
| 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.
| thewebcount wrote:
| > 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?
| wanderingmind wrote:
| 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.
| tyler_mann wrote:
| 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.
| dnautics wrote:
| 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.
| avip wrote:
| I'd pay $$ just to have someone reviewing our Raman bowel
| of a codebase and documenting it.
| all2 wrote:
| > and documenting it
|
| Sounds like you need two or three people dedicated to the
| task. Documentation is a whole profession by itself.
| Well, _good_ documentation.
| mncolinlee wrote:
| 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.
| ryanbrunner wrote:
| 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.
| Too wrote:
| 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.
| andy_ppp wrote:
| 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?
| mncolinlee wrote:
| 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?
| nick007 wrote:
| 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.
| kevingibbon wrote:
| 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.
| walkingriver wrote:
| 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.
| gavinray wrote:
| You do this on the side of your dayjob for extra income?
| mncolinlee wrote:
| 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.
| koblas wrote:
| 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.
| FullyFunctional wrote:
| 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").
| 6510 wrote:
| No one has to die or quit. You could just hire a new dev who
| doesn't have to wait for someone to explain the relevant
| parts of the godzilla object.
| mytailorisrich wrote:
| 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.
| walkingriver wrote:
| 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.
| z3t4 wrote:
| > Unlimited lines.
|
| I wonder how long it will take them to go though my entire code
| base
| antonpuz wrote:
| I've greatly improved as a developer by reviewing code, this
| improved my project and improved team alignment, that's much more
| valuable than getting inputs from human linter.
| booleandilemma wrote:
| And I'm guessing their reviewers work for free, like on Code
| Review Stack Exchange?
|
| Or are these people working for relative poverty wages overseas?
| senko wrote:
| > And I'm guessing their reviewers work for free, like on Code
| Review Stack Exchange?
|
| No need to guess, it takes 20 seconds to check:
| https://app.pullrequest.com/signups/reviewer
| GrahamCampbell wrote:
| Reviewers are well-compensated, meaning pullrequest.com can
| attract very high quality reviewers.
| walkingriver wrote:
| 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.
| joshgrib wrote:
| Can someone give a solid and succinct review of doing this as a
| side-job? What was 1) the hourly pay, 2) language(s) you
| reviewed, and 3) how was the work?
| racl101 wrote:
| Human linter?
| game_the0ry wrote:
| How is this not the same as contracting a freelance dev? You
| might as well contract through wipro / infosys / tata / cognizant
| / hcl.
|
| I am annoyed at how replacing software engineers is somehow
| viewed as a business problem that can be solved as a SaaS
| business or no code tooling.
|
| PR's customers need to hire more developers and pay them well. It
| is expensive and hard, but that is the market business people
| need to accept.
| srhaegrfes wrote:
| 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?
| cptaj wrote:
| I've never seen outside consultants make useful contributions to
| a project. This is even worse.
|
| Thanks, I hate it.
| pelasaco wrote:
| 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?
| Recursing wrote:
| From the FAQ:
|
| > 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.
| pelasaco wrote:
| > Reviewers earn anywhere between $50 and over $3,000/week.
|
| 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?
| lambic wrote:
| Why don't you say what you're thinking instead of
| disguising your opinions as questions?
| yjftsjthsd-h wrote:
| 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.
| breakfastduck wrote:
| If you read the rest of this thread you'll see numerous
| reviewers from the site giving their 2 cents. Much
| preferable to just making assumptions.
|
| 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.
| hunterb123 wrote:
| He's got a point.
|
| $50-3000 / week tells you nothing.
|
| They should tell you the avg $ per hour.
| mgkimsal wrote:
| 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.
| scubbo wrote:
| 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.
| pelasaco wrote:
| $3k/week is not a metric, if you don't define a week. 40
| hours? 168 hours?
| wodenokoto wrote:
| When I saw the 200/hour pricing I thought "that actually sounds
| reasonable", but how is 699/month for unlimited review going to
| work out?
| cannonpalms wrote:
| 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.
| danuker wrote:
| Well, something has to give. Perhaps the devs work 4h/mo, so it
| is just a slight discount compared to the hourly rate.
| ivanche wrote:
| "All of our employees, contractors, and reviewers are based in
| the US and Canada. "
|
| https://www.pullrequest.com/faq/
| walkingriver wrote:
| 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.
| adamqureshi wrote:
| Im a 1 MAN SHOP and i hire contractors ( engineers). I am not
| sure if this a good fit for me? My main concern is of course
| cost. Do you provide feedback on what todo with the software
| stack? ( can my guys implement a new plan provided by you?)and
| 2nd question is do you provide pay for service, for example if i
| am interested in a new software architecture and my guys just
| code it up / follow your plan / recommendation. Thx
| speby wrote:
| Gosh, building any tools that target developers as customers is a
| gargantuan task. Kudos to you for attempting something new here
| and giving it a whirl. We developers are some of the harshest
| critics around, no matter how legit or unfounded our criticism
| may be. Keep it up!
| soheil wrote:
| I lot of people are saying CR is a way to transfer knowledge
| within a team. I think they're missing the point. Sure, you can
| transfer knowledge within a PR but don't you think that's a bit
| late? By the time CR is requested, the code has been written, the
| architecture has been thought of and so much time has been wasted
| doing the "wrong" thing. It's a horrible way to learn/train.
| 8organicbits wrote:
| 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.
| smalter wrote:
| i've used pullrequest a bunch and had great experiences with the
| product and company.
|
| mostly used it in small companies with 1-2 devs where it helps to
| get another pair of eyes on it. also, helpful as career
| development / learning for a junior dev.
|
| happy to answer any questions.
___________________________________________________________________
(page generated 2021-12-20 23:00 UTC)