4 ms·

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

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.

blacklion 5y ago
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 5y ago
  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).
alkonaut 5y ago
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 5y ago
  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 5y ago
  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 5y ago
    > 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 5y ago
without context you can't catch subtle mistakes
mbesto 5y ago
> 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
fourseventy 5y ago
Not very realistic for the real world
sbassi 5y ago
that is impossible in lot of cases

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.

blacklion 5y ago

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

alkonaut 5y ago

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

without context you can't catch subtle mistakes

mbesto 5y ago

> 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

fourseventy 5y ago

Not very realistic for the real world

sbassi 5y ago

that is impossible in lot of cases