What to look for in a PR review

Top comments (5)

Sort discussion:

• Most upvoted and relevant comments will be first

• Latest

  Most recent comments will be first

• Oldest

  The oldest comments will be first

Collapse Expand

Alexandru-Dan Pop

I'm Software Engineer who loves working with Javascript, React, and Node.js. 👨‍💻 I'm also a big Typescript fan! 🚀

• Location

  Cluj-Napoca

• Education

  Technical University of Cluj-Napoca

• Work

  Software Engineer at Freelancer

• Joined

  Apr 1, 2020

• May 19 '20

• Copy link

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 Author

Software Engineer. Former digital nomad at Toptal. Open sorcerer. Thoughts on career growth, remote work, and web dev.

• Joined

  Jun 28, 2019

Author

• May 19 '20

• Copy link

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 Author

Software Engineer. Former digital nomad at Toptal. Open sorcerer. Thoughts on career growth, remote work, and web dev.

• Joined

  Jun 28, 2019

Author

• May 18 '20

• Copy link

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

Cofounder of Bytes Robotics. Passionate about creating products that improve quality of life and integrate seamlessly into everyday environments.

• Location

  Carmichael, CA

• Education

  UC Berkeley MET Program - Dual Degree EECS + Business

• Work

  Cofounder at Bytes Robotics

• Joined

  May 20, 2020

• May 21 '20

• Copy link

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 Author

Software Engineer. Former digital nomad at Toptal. Open sorcerer. Thoughts on career growth, remote work, and web dev.

• Joined

  Jun 28, 2019

Author

• May 24 '20

• Copy link

All in for ripping off one-liners here too!