The simplest change possible should produce a better debate than LGTM
Looks Good To Me. LGTM 👍 You’ve seen this on a pull request that you’ve authored, and perhaps you’ve sent this as a comment yourself.
No matter how technically simple a change is (e.g. a one-line of code diff that simply changes a string) there should be at least some minimal discussion about that change. Does the text change look good to you? Why does it look good to you? Why do you think that text will convert better? What text has converted better before? Even something as simple as a text change can open up a whole debate about user value generation via code (code doesn’t have intrinsic value). There’s always something more thoughtful than lgtm to say. “I tested with this set of steps and lgtm”.
However, there’s another bad example at the other extreme. We’ve heard horror stories at big companies where a simple text change has taken up to 6 months. This isn’t what you should do either. Just briefly explain why it looks good or bad to you.
The LGTM Paradox
Developers spend 30% of their time doing code review but 65% of pull requests get rubber stamped with LGTM. There’s a paradox here.
It seems like developers thoughtfully review code, but they are not teaching others about their process which leads to information osmosis problems across the team.
The problem with rubber stamping with LGTM
Code review is a learning opportunity for everyone. In companies with the best engineering cultures everyone does code review. Juniors, engineering leaders, and even non-technical stakeholders such as product managers and product designers who bring a more holistic view to the process.
Not doing code review is detrimental to developer velocity in the long term. Let’s expand on this.
First, the risk of not deploying code that satisfies the user’s requirements to production exists. Whether this is because some app flow broke with wrong code, or because there’s some implementation that’s technically correct but not what the user wants, or because a potential security vulnerability is being introduced. If you are not thoughtfully reviewing code and just saying lgtm you are exposing your codebase, product, and company to this failure mode.
Second, you are not learning about the codebase. If a code refactor happens on a pull request, this is something that should bring up a lot of questions to you: Is everything still working as expected? Does it bring unexpected breaking changes? Is this refactor necessary, or is it over-engineering the codebase? To be able to keep adding features with this new approach, what should the method be?
Finally, if you just post lgtm on a pull request you are failing to mentor other developers. You are not teaching people about how something specific on the codebase works, nor about the organization’s coding standard and style, nor about what produces value to the user.
The PR comment/approval ratio metric as a solution
We strongly recommend engineering leaders tracking this metric.
A developer hasn’t approved PRs? The dev isn’t doing code review, that’s bad, and that should trigger a conversation. A developer has a lot of PR approvals but no comments? The dev is rubber stamping with lgtm. That’s also bad, and should also trigger a conversation. A developer is posting a lot of thoughtful comments on PRs, and also doing PR approvals? This dev should be rewarded.
Engineering leaders should track this metric to avoid rubber stamping pull requests with lgtm but as any other metric, it can be gamed. We recommend measuring it, and stating on a strategy document that it is measured, but not being very vocal about its measurement.
Quick solutions to the problem
There are certain low hanging fruits to check for when reviewing a PR that can easily start a conversation. The low hanging fruits to look for, which can easily replace that dreaded lgtm comment are:
- Is the code commented?
- Are variable and function names self-descriptive?
- Is there unnecessary code logging?
- Are data types in place? (This depends a lot on the programming language).
- Can the code be refactored into smaller functions?
The solutions to an engineering culture that rubber stamps
Exceptional engineering leaders give psychological safety to their team.
There is no such thing as a stupid question, and there is no fear of questioning a certain technical approach. It all starts by having this cultural DNA of respect on your engineering leadership team.
Devs also rubber stamp with lgtm other people’s pull requests because they are pressured by a deadline to ship certain code themselves, which goes against carefully and thoroughly reviewing other people’s code. Just know that deadlines are arbitrary anyway (especially when doing innovation), and that there’s always a tradeoff between speed, cost, and quality where you can only choose 2 out of the 3 at max.
We also built a GitHub app that automatically traces code context for pull requests and posts such context on a comment.
Top comments (0)