You know the deal; Code Reviews should focus on the big-picture. The relevant-deep issues related to software architecture while avoiding the code style, formatting, and subjective opinions...
...in theory, that is
I mean, the important stuff should take precedence. Here are a few things that come to mind:
- Consider breaking changes: will these changes cause chaos in the current system?
- Keep a consistent API: will these changes cause chaos in external systems?
- Does it solve the thing? (is it correct?)
- Security issues?
- Concurrency issues?
- Double check when adding new dependencies
- Do you need to update the documentation?
- Do you need to update the test coverage?
- ...
I am not saying the opposite.
But subjective things count too.
At least while other people is going to read and maintain the code base (not sure what's to happen within 10 years 🤖), let's not minimize or ignore their opinions regarding readability.
Most of the time there is no objectively better naming or a unquestionable better way of organizing a module.
What's crystal clear to one coder, it's a Lovecraft horror to another 🦑.
The real problem is, this sentence:
"this is more/less readable"
is a matter of perspective.
It's not that readability isn't important; it's just hard to measure "how readable" a piece of code is.
Here is a piece of advice - a tip if you want - that works for me:
- ❌ Avoid pointing out just what you don't like;
- ✅ DO write down the alternative you'd prefer.
Since all of this is subjective and revolves around reading preferences, please make sure to articulate your choice thoroughly. Reading actual code - instead of imagining the alternative - may lead to better comprenhension
Ah! and the closer to real code, the better (I mean, try to avoid asdasdads-ing).
Write actual code.
On GitHub, there is a wonderful «make suggest» button:
This is going to wrap your co-worker's line with suggestion special mark (GitHub flavored markdown).
This is a dummy example of a rename request; let's say I'd prefer to name this particular method showError() instead of setApiError(). And take the chance to rephrase the typing declaration.
Both are minor, subjective change requests.
Neither the rename, neither the type declaration may be tagged as "important stuff".
My point is that offering the actual alternative makes easier to evaluate if this minor change is acceptable.
The idea is: if both programmers agree on this minor thing... why not changing it?
...otherwise, do NOT start a renaming discussion. Again, this is not on the "important stuff" list. Do not spend more time on this matter.
You proposed a quick suggestion, move on.
If you're not coding on GitHub (or just that the suggestion thing isn't your cup of tea) what I was doing until they released this button was using a diff code block:
Images generated by leonardo.ai
Thanks for reading 💚.
Top comments (0)