4
glowFX
43d

If you are reviewing pull requests to your code base, what aspects do you focus?
I am asking, because a colleague of mine most of the time complains about minor code style issues, like the following:

"why is that method static, it belongs to the object. I am not used to static methods, please adjust"

"the methods are not arranged in a standardized breadth-first order"

"Please use that other kind of initialization..."

Those nit-picks make me crazy sometimes. Is it just me?
I mean, her reviews also contain valuable content, but those small complains about coding style feel like "I did not found anything else but I have to". ^^

Comments
  • 2
    > Is it just me?

    No. I tend to focus more on error handling+logging.

    When you access an external resource (web api, database, etc), are you logging enough information to troubleshoot the issue *when* it fails? What do you want to tell your future self about this area of code, because 6 months from now and a phone call at 3:00AM, you won't remember any of this.

    Ex. This morning our access to Oracle cloud was broken. The only error logged was the JSON serialization exception. No context, nothing to indicate that service was the problem. Took about an hour for the dev to figure out it was a 404 response. He didn't ensure a 200 response before trying to deserialize. Oops.

    If he had logged what the actual response was, the URL resource, he could have figured out the problem in about 3 seconds (and keep about 6 other techs from scrambling around pointing fingers at each other)
  • 1
    I mean, unless you have a good reason to not do it like the review says, than It's likely not a bad thing, though if you have a good reason it should be discussed.

    The point of reviews is to improve code quality, and one could argue that accommodating a style that most devs in the team use is the higher quality standard, unless It's objectively wrong (like going against well known best practice)

    This should be a colaboratory process and code should be discussed openly without shame or ego to benefit the project overall, both short and long term
Add Comment