What do you look for in a PR review?
Carlos Roso

Carlos Roso @caroso1222

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

Joined:
Jun 28, 2019

What do you look for in a PR review?

Publish Date: May 18 '20
22 5

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

Comments 5 total

  • Carlos Roso
    Carlos RosoMay 18, 2020

    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?

  • Alexandru-Dan Pop
    Alexandru-Dan PopMay 19, 2020

    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
      Carlos RosoMay 19, 2020

      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!

  • caelinsutch
    caelinsutchMay 21, 2020

    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
      Carlos RosoMay 24, 2020

      All in for ripping off one-liners here too!

Add comment