What do you look for in a PR review?

What are you strict about when reviewing code? What do you always point out in your peer reviews?

Comments

[Alexandru-Dan Pop]

Code structure & common patterns used

Is the code structured correctly and following the same trends and patterns as the rest of the application?
If it breaks from those patterns is it a good reason to do so?

Code quality

Does it follow good namig conventions?
Are FP principles used?
Are unit or integration tests are present?
If existing code is modified and it had automated tests to cover it, have the tests been updated to also check the new logic?

Maintainability

If i think certain parts are badly written or too complex, unreadable, or could be simplified I might suggest alternatives.

Note to self when reviewing code

Can you automate more to make some general mistakes occur less often?

• avoid formatting comments by using a code formatter
• avoid some code quality comments by using linters
• discuss more upfront when a colleague has a challenging task so the outcome is more predictable (have a huddle task for complex work)

[Carlos Roso]

Great learnings from here. I like how you strive for codebase consistency (is this similar to what we have elsewhere) and also for automation (avoid style discussions by introducing a common linter). Thanks for sharing!

[Carlos Roso]

FP
I think I'm biased but, after learning about functional programming 2 years ago, I'm always looking for ways to make code more functional.

So whenever I see functions like this...

function doSomething(list, idx) {
  if (idx > 0) list[idx]++; // avoid side effect
  return list[idx]
}

... I would point out how functions should have the least amount of side-effects and find a cleaner way to achieve this outside of the function.

Comments
I can't disagree more with the statement "good code should document itself". I do find value in a comment given I'm not the only one reading this code. If there's a routine that takes my brain 3 minutes to understand, I'd ask OP to put a comment on why this piece of code exists.

Error handling
This is a very common pitfall I see on PRs. Devs accounting for success paths only. Read the DB and return the value; cool, but how are you handling errors? create object and route to /dashboard; cool, but what happens if the object can't be created?

[caelinsutch]

Readability
I don't really care how many one liners you have, if they're not readable, they aren't useful. Inline comments, correctly named variables, and DRY code is my number one thing.

Lint it please
I'm a little OCD about this, but I hate when other programmers don't lint their code :( Makes me really annoyed.

Top Level Documentation
Each file should have some JSdoc quality explanations of the file, I should be able to quickly read through a summary to see what each method and class does.

[Carlos Roso]

All in for ripping off one-liners here too!