I’m very comfortable with reviewing the code I own. I know the logic, the edge cases and the data flow. I attended (and contributed to) the architectural discussions related to the topic.
Hence I can evaluate not the code sensu strictu, but:
- how it solves the issue,
- how it fits into existing high-level conventions (e.g., regarding the data flow),
- how it aligns with our future plans (related to both, feature development and maintenance).
If I comment such code, my comments are valuable.
I remember also reviewing the foreign code, where I wasn’t that familiar with the domain/logic. None of the list above applied right away. Of course, after some digging/investigation I could grasp it but it required time (which, being honest, I not always had).
On the other hand, a strange feeling persisted: wouldn’t it look suspicious if I simply LGTM1 it without any note?
The obvious way of coping with impostor syndrome was nitpicking, thorough commenting over the variable names and using of newest language syntax.
How much value bring such comments?
At what price do appropriate changes come?
Having these things in mind, helped me to focus on the gist and to accept that I’m not always grasping it from the first glance. It’s ok, it takes time. This was really hard to accept though…
Indeed, one of the goals of code reviewing is to spread the knowledge and this never happens fast.
✏️Note
✅ Yes, it’s ok if you don’t understand the whole logic in a foreign merge request.
✅ Yes, it’s ok to approve it. This fairly reflects your position: “Looks Good To Me”. 🙈🙉🙊
💩 No, you should not prevent the MR from being merged due to “if vs. ternary” comments.
👉Tip
I still follow the “see issue — report issue” practice. So I still comment on typos or unclear variable names.
But I tag these comments as “#minor“ . We agreed in the team that it’s fine to ignore/close such comments if we are short on time.
References
-
LGTM — Looks Good To Me.
The only comment one leaves on 5,000 lines merge request😅 ↩