5 ms·

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

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

adamnemecek 5y ago
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.
- noduerme 5y ago
  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?
  - alexdowad 5y ago
    Well, maybe the people you have available to review code aren't all that good. Nobody ever wants to admit that their team is below average, but 50% of teams are. Maybe due to limitations on team size, it's not possible to have an in-house expert on every technology used.
- tialaramex 5y ago
  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)
gitfan86 5y ago
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 5y ago
  Vitamin, not painkiller. Just keep that in mind
- tialaramex 5y ago
  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.
danuker 5y ago
> 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 5y ago
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.
- yosito 5y ago
  Typically, it is part of the senior engineer's job to provide "code review as a service". An engineer who can't review a junior's code isn't a senior.

adamnemecek 5y ago

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.

noduerme 5y ago
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?
- alexdowad 5y ago
  Well, maybe the people you have available to review code aren't all that good. Nobody ever wants to admit that their team is below average, but 50% of teams are. Maybe due to limitations on team size, it's not possible to have an in-house expert on every technology used.
tialaramex 5y ago
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)

gitfan86 5y ago

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 5y ago
Vitamin, not painkiller. Just keep that in mind
tialaramex 5y ago
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.

danuker 5y ago

> 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 5y ago

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.

yosito 5y ago
Typically, it is part of the senior engineer's job to provide "code review as a service". An engineer who can't review a junior's code isn't a senior.