Bike shedding is a real thing. I think about it a lot when reviewing code. I try to see the big picture and ignore small nitpicks in favour of reviewing the overall architecture and intent of a change.
But sometimes feedback you might think of as bike shedding is actually super useful. One of these cases is when it takes a while to understand the code. If it is hard to understand someone’s code, it can be tempting to think it’s a problem with you (oh, hey there imposter syndrome!): maybe you’re missing context, or you just aren’t smart enough.
I say no: as a code reviewer, you are closer to the problem than anyone else who reads this code later. Assuming there is an adequate PR description, and you read it before looking at the code, you should have more than enough context to understand the code without needing to re-read it twenty times or ping the author directly for an explanation. Someone who comes across this code a month, a year, or 10 years later (including the author themself) will definitely have a far harder time than you are having now, so do that person a favour and call it out!
If you find yourself reading and re-reading code during a review, let the author know that it was hard for you to understand! They may just have been working on the problem for a while and didn’t realize how it looks to someone lacking their expert context. They may have tried to simplify it and couldn’t figure out a way. Maybe the solution is as simple as improving the PR description (I think a detailed PR description documenting the context around a change is a great way to communicate context). Maybe it’s as simple as making the code more verbose. Regardless of the solution, if you’re having a hard time reviewing someone’s code, that is always great feedback.
- “I had a lot of trouble understanding what is going on here. Could this be easier to understand if we broke this branch condition into multiple variable assignments?”
- “It took me a long time and lots of jumping around between files to figure out what is happening here. Is there an abstraction that could make this easier to understand?”
- “I’m not sure what is happening here. It looks like <…>. Is that correct? Maybe I’m missing some context”. If it turns out that you are missing context, you can suggest updating the PR description.
No matter what, make sure to always assume positive intent, and leave comments with positive intent! We’re all doing the best we can!