4 ms·

You should attack it. You should pick it apart and try to break it in as many ways as possible. Of course you should meter your effort depending on how critical

You should attack it. You should pick it apart and try to break it in as many ways as possible. Of course you should meter your effort depending on how critical the changes are. It amazes me the lackadaisical attitude towards code review. "It will probably be okay" is the worst attitude to have with computer code. It is the most brittle, brutal environment imaginable.

all2 5y ago
Can you expand on this?
- kmtrowbr 5y ago
  You should pull the PR you're reviewing down into your IDE and you should spend like, an hour reviewing it carefully and trying to break it. Have a mental checklist: say they renamed a function ... is the file named the same as the function? Did they remember to rename the file? Did they delete parts of the code? Did they remember to delete everything related to that? Often the most challenging parts are negative: it's harder to remove properly than it is to add new things. Say they changed the validation on a model: is this going to work well with the production database? Do we need to write a migration to catch the database up to the new reality of the code? Automated tests are just automated warning flags that give you confidence that the codebase is basically hanging together. You still need to be very careful & thoughtful as you're making changes. I like to actually reset the branch so I can see all the raw changes in my IDE, this helps me to think it through, in git you can do this via: git pull feature-branch; git checkout develop; git checkout -b my_initials/feature-branch; git merge feature-branch --squash --no-commit
  - efficax 5y ago
    An hour is way too much time. If you need that long the pr is too big and needs to be broken up into smaller parts
  - QuercusMax 5y ago
    How big are these changes? That sounds horrendously slow. If you don't trust your tests, write better tests. If you don't trust your migrations, write better tests.
    - Dunedan 5y ago
      In probably most cases the person who does the code changes does write tests for it as well. So as a reviewer without understanding what the code and the tests do, how do you want to assess if the tests actually do something useful?
      - QuercusMax 5y ago
        If your code and tests are so opaque that the reviewer can't understand them without physically running and modifying the code, then as a reviewer you should ask the author to make it clearer. Because otherwise that's how you get write-only code. Things like code style and documentation can make a big difference, and review time is when you can help enforce them and build team culture and knowledge.
LeftHandPath 5y ago
It's something I've noticed in modern peer-review practices, too. And some have gone as far as creating fake papers to study whether or not they get published: https://www.theatlantic.com/ideas/archive/2018/10/new-sokal-hoax/572212/ https://www.theatlantic.com/ideas/archive/2018/10/new-sokal-... There's a human instinct towards laziness. If there are no outwardly-visible reasons to distrust something, it doesn't get evaluated. It's why a hard-hat and a clipboard can get you backstage, and why you don't spend 2 minutes scanning every bush for crouching predators before you walk past it. In everyday life, a "lazy" tendency saves time by avoiding needless work. In engineering, academia, etc., it can create dangerously low standards.

all2 5y ago

Can you expand on this?

kmtrowbr 5y ago
You should pull the PR you're reviewing down into your IDE and you should spend like, an hour reviewing it carefully and trying to break it. Have a mental checklist: say they renamed a function ... is the file named the same as the function? Did they remember to rename the file? Did they delete parts of the code? Did they remember to delete everything related to that? Often the most challenging parts are negative: it's harder to remove properly than it is to add new things. Say they changed the validation on a model: is this going to work well with the production database? Do we need to write a migration to catch the database up to the new reality of the code? Automated tests are just automated warning flags that give you confidence that the codebase is basically hanging together. You still need to be very careful & thoughtful as you're making changes. I like to actually reset the branch so I can see all the raw changes in my IDE, this helps me to think it through, in git you can do this via: git pull feature-branch; git checkout develop; git checkout -b my_initials/feature-branch; git merge feature-branch --squash --no-commit
- efficax 5y ago
  An hour is way too much time. If you need that long the pr is too big and needs to be broken up into smaller parts
- QuercusMax 5y ago
  How big are these changes? That sounds horrendously slow. If you don't trust your tests, write better tests. If you don't trust your migrations, write better tests.
  - Dunedan 5y ago
    In probably most cases the person who does the code changes does write tests for it as well. So as a reviewer without understanding what the code and the tests do, how do you want to assess if the tests actually do something useful?
    - QuercusMax 5y ago
      If your code and tests are so opaque that the reviewer can't understand them without physically running and modifying the code, then as a reviewer you should ask the author to make it clearer. Because otherwise that's how you get write-only code. Things like code style and documentation can make a big difference, and review time is when you can help enforce them and build team culture and knowledge.

LeftHandPath 5y ago

It's something I've noticed in modern peer-review practices, too. And some have gone as far as creating fake papers to study whether or not they get published: https://www.theatlantic.com/ideas/archive/2018/10/new-sokal-hoax/572212/ https://www.theatlantic.com/ideas/archive/2018/10/new-sokal-... There's a human instinct towards laziness. If there are no outwardly-visible reasons to distrust something, it doesn't get evaluated. It's why a hard-hat and a clipboard can get you backstage, and why you don't spend 2 minutes scanning every bush for crouching predators before you walk past it. In everyday life, a "lazy" tendency saves time by avoiding needless work. In engineering, academia, etc., it can create dangerously low standards.