I mostly like this. When I'm reviewing I want to see atomic changes with comments and verbose commit messages.
I do take issue with one point, since I've been on the receiving end of a reviewer who wants this:
> Explain alternative solutions for the same issue
For a complex problem, there is a large space of possible solutions. The idea of either exhaustively checking every possibility or else choosing some random possibilities for scientific experimentation both strike me as ridiculous.
When I'm working on a change, I usually start with an existing implementation from somewhere else in our code which solves a similar problem. I massage it into place, removing what isn't relevant to the new problem, and changing what needs to be changed to make sure it does what is required while passing all tests.
That's it. It's not a scientific process. There are no multiple possible implementations to consider. There is the implementation which I arrived at through organic means, and it should be reviewed on its own merits.
So when a reviewer asks me why I don't do it some other way, and they don't suggest anything about what other way might be better, I just feel like throwing my hands in the air. I did it the way I did it because that's the way I did it.
If someone thinks I should do something differently, I believe there is some onus on them to either explain what is wrong with my approach, or motivate a better alternative, or they should take the work off my hands and do it themselves so I can focus on something else.
Everything else feels like unnecessarily putting the brakes on due to FUD. The job of the reviewer is not to hold back a change due to wanting to navel-gaze over the infinite possibilities. The job of the code reviewer is to review code.
Thanks for sharing your thoughts and continuing the discussion on reviews. I agree that the reviewer should be precise and constructive with their comments.
Thanks everyone for the super insightful discussion! I have learnt a lot from the comments, and I have also updated my blog post accordingly. You can find the updated version at the same link.
Thanks for that (and no thanks for this)! You won't find too many this's (or deeply nested properties of any kind) in my own decade+ long adventure with JavaScript.
Unrelated but you can also block ads with the FF mobile browser using ublock origin. I understand playing videos with screen locked is a big plus for brave though.
I do take issue with one point, since I've been on the receiving end of a reviewer who wants this:
> Explain alternative solutions for the same issue
For a complex problem, there is a large space of possible solutions. The idea of either exhaustively checking every possibility or else choosing some random possibilities for scientific experimentation both strike me as ridiculous.
When I'm working on a change, I usually start with an existing implementation from somewhere else in our code which solves a similar problem. I massage it into place, removing what isn't relevant to the new problem, and changing what needs to be changed to make sure it does what is required while passing all tests.
That's it. It's not a scientific process. There are no multiple possible implementations to consider. There is the implementation which I arrived at through organic means, and it should be reviewed on its own merits.
So when a reviewer asks me why I don't do it some other way, and they don't suggest anything about what other way might be better, I just feel like throwing my hands in the air. I did it the way I did it because that's the way I did it.
If someone thinks I should do something differently, I believe there is some onus on them to either explain what is wrong with my approach, or motivate a better alternative, or they should take the work off my hands and do it themselves so I can focus on something else.
Everything else feels like unnecessarily putting the brakes on due to FUD. The job of the reviewer is not to hold back a change due to wanting to navel-gaze over the infinite possibilities. The job of the code reviewer is to review code.