Peer reviews are an arguably essential way of ensuring quality in any software engineering team, but only if they’re done well. In my experience, this is rarely the case — consistently good reviews, and indeed reviewers, even in fairly functional teams, are hard to come by. At best, chronic poor reviewing can lead to the whole review process becoming a worthless burden, and at worst serious errors can be missed.
LGTM 👍🏻
This is not a review. This is a cop-out.
I have seen this acronym be the sole feedback in a pull request more times than I can count. I’ve seen it for a one-line change to a README, for a complex and business-critical new feature, and everything in between. For me, it’s a fantastic example of how not to review constructively, and it seems to have infected pretty much everyone in the current software community at one time or another; myself included.
“What’s wrong with it?” you might ask. Simply, it provides no value.
“Well, duh!” you might say, but I’m still betting you’ve used it at some point.
To explain why LGTM and its kin provide no value, all you need to do is ask yourself, “Why do we review?”.
There really is only one right answer to this, and no, it’s not, “Because otherwise I’m not allowed to merge”. It’s to ensure something satisfies its expectations before being accepted.
This means that in order to review something constructively, you need to be able to demonstrate that you understand what something is or does, and more importantly, what it should be or do. LGTM cannot demonstrate either, so it provides no value in a review.
To step back for a second, why does a one-line change to a README require a review in the first place, really? The answer is almost certainly that it doesn’t. More often than not, the only reason a review like this is requested in isolation is just that the software development process or tool mandates that a review is done. OK, so it’s unnecessary, but fine… just give the README a glance to confirm the typo is resolved, add a note to say that “misspell” is now spelt correctly and press the “Accept Changes” button. Nothing else really demonstrates the fix has been checked, yet inexplicably the reviewer often feels compelled to instead just sign off with a token LGTM, like they’re filling an awkward silence.
If LGTM had somehow become the de facto acknowledgement of a fixed typo in the software community, it would kind of be OK – it would have organically evolved into a gesture. But it hasn’t, and yet it’s still all too often used for reviews both big and critical and small and pointless, making it impossible to tell whether the thing being reviewed has been reviewed at all!
I’ve used LGTM as the lightning rod for poor reviewing, but it’s certainly not the only way to do it; it’s just a glaring example. Fundamentally, to review constructively you need to be able to demonstrate both an understanding of the thing being reviewed and its brief. Otherwise, what’s the point?
There is no one correct way to perform a good review – it is a matter of context, forum and personal style. But the principles of reviewing constructively and what constitutes a good review apply ubiquitously, and the consequences of a bad one can be disaster.
