We've all been there. You've got comments on your code from a reviewer and it feels like you might as well rewrite everything from scratch. It's demoralising and feels like a waste of time, especially since you know it works and fulfils the requirements of the task.
Situations like this can lead to you cherry picking your reviewers, having endless back and forths about a forEach vs a for, and generally feeling a bit fed up with the whole process.
Well, let me introduce you to a system that lets the reviewer express their thoughts in a manner that indicates exactly how important each comment is and how much attention you need to pay to it.
MoSCoW, taken from the world of project management is the name given to a list of requirements laid out as:
- Musts
- Shoulds
- Coulds
- Woulds
I won't dwell on what they mean exactly in the world they originate from because it's not relevant and the system is so self explanatory you've probably already worked out where I'm going.
M/S/C/W
Each comment should begin with M:, S:, C or W: and then the comment.
- Must: this has to be changed to be approved. These are errors that either go against the coding standards of the team or are clearly just incorrect and will lead to a bug. These can't be ignored.
- Should: these are things that would be an obvious and clear improvement. You have to give a reason to ignore this and the code reviewer has to agree. A third party can be brought in if needs be.
- Could: these are for niceties that the reviewer thinks would improve the codebase in some way but are probably going above and beyond what the task requires. If they have an idea for a slightly better function name or they want to leave a comment but aren't all that bothered if you take the advice then this is the prefix to use. These can be ignored without a reason given.
- Would: these are reserved for the "I wouldn't have done it this way" comments that require lots of rework but are 100% personal preference. These rarely get used because a would is really just the reviewer being given space to express their thoughts. An ideal usage of this is to educate a junior member of the team and perhaps follow up with a conversation around it post review.
In the example above about a for loop I would say:
S: Use forEach here to avoid off by one errors and aid understanding of loop conditions
or similar.
Summary
Introduce this at your next team meeting, get a clear understanding of your musts and a rough idea of some shoulds and you will see how liberating this is for everyone involved in the code review process.
Want more like this? Sign up to my mailing list at Career Switch To Coding and get a free chapter from my book.
Oldest comments (6)
Cool post !
I like the idea and will suggest it to my team, thank you for sharing 👍
I like the idea of this! I think that having a standardized framework for giving feedback like this also takes away a bit of the "nagging" that PR comments sometimes are perceived as, without having to resort to "this is a nitpick but...".
It also makes PR's machine readable, if that's something that tickles your pickle!
Nice, we started using this two months ago at our company!
It's working quite well I have to say. And it is liberating, much better than other techniques we've tried.
Nice! We've started using "conventional comments" at work but this looks like a potential alternative too!
It's great to see a way to make sure review comments are marked as how important they are. I know a common complaint with reviews is not knowing if something is a required change or not.
We do something similar on our team, but instead just say if something is personal preference if it is something that isn't objectively and easily provable as better, a standard violation or a bug. The personal preference comments can be ignored, but a reason always need provided so the devs have to show they at least considered it. But the reason could be a simple as "I like it this way for x reason"
We tried this out in my previous team and I think it worked very well!