DEV Community

loading...

Discussion on: What do you look for in a PR review?

Collapse
caroso1222 profile image
Carlos Roso Author

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?