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". ^^

seems fine. she's kind of selfish & newbie but that's not problematic. seems nice to me

by selfish I mean she can state where she's at and she's honest about it. I don't mean it negatively. current culture views it negatively but what it means is you can trust her, and it would be very easy to know where she's at if arguments transpire because she will disclose information about herself honestly whereas most people in debates / arguments focus on winning and dunking on someone which makes conversations with them unproductive

you could teach her static methods and she might appreciate that

lol she doesn't know what to call different initializations. yeah she's kind of one of those accounting types... fuck. she's Fi-Te in mbti typology. probably INFP. but they're not terribly smart with logic (no Ti which is like magical and quick thinking that you can't directly express). so they focus on explicit simple logic and seem to understand code in their head as if it's copy-paste

> 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)

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

@jestdotty

> but what it means is you can trust her, and it would be very easy to know where she's at if arguments transpire

Oh, thats a good one. Haven't seen it like that. :)

@Hazarth I somehow like the different coding styles in our codebase. Sometimes I can even see from the style, who wrote it. And I think that's fine.

So I avoid discussions about those tiny style differences, because they are just a waste of time. That's why I also just change such tiny changes if the reviewer complains about it, even if I am annoyed by them.

Of course there are bigger bloppers, like +200 line methods with +4 nested loops I start to complain about in PRs. So there are limits in my tolerance. :)