Strange, I’m on a team rn with people who just approve everything.
I’m craving some constructive feedback.
I’m craving some constructive feedback.
Instead of "please write this code, submit a pull request, and I'll tell you what you did wrong," I have recently been saying to new coders "take a shot at this problem, and when you've got something working let's get together and refine it, there is some context it will take time to learn so we'll probably need to make a lot of changes or even rewrite it before release."
That way they know what's coming (a lot of changes), they know why it isn't a sign they're inadequate, and it's more about working together than passing judgment.
It has also helped reduce the massive delays while junior developers agonize over making something perfect.
I've been on the receiving end of enough toxic code reviews to develop an aversion to do them. So when asked to do them, I really just ask, did you test it?, do you want to demo it to me?, then approve it.
When people demo to me, I can see their pain points, because they always point them out. "Oh yeah, I was having trouble getting this to work, as you can see, it's still a little slow" and it gives them an opportunity to ask for advice or merely to vent.
So if you want good, honest feedback, a demo is the way to go. Honestly, it's hard to really grok what a bit of code is doing just by reading it. And who has time to pull code and run through it solo when doing a review?
I mean, sure it doesn't render well as HTML, but its readable with the code and the rest of the project has none so...
(It might also mean your team doesn't care about anything any more, in which case look for a better opportunity, unless you want to just cruise along for a while for whatever reason, in which case turn into a bare minimum performer like the rest of your team).
I'm a senior engineer in a known company and my target is always to leave no comments on a patch. I only give question-comments if I absolutely must know something before merging, or please-fix comments if something is definitely wrong and I can prove it. Also, if it's a part of the system that poses little risk (for example, if it breaks I can assign the bug back to the author and patiently wait for a fix with no serious damage) my tolerance for bugs greatly increases. The last part I'm not too proud of but frankly I don't have time to review everything. A "fix" for the last part would be to find a lower rank reviewer-buddy who has more time to share knowledge.
One of my peeves is a review that points out a bug/incorrectness without offering proof, or suggesting the fix/correct way. The reviewer might observe a code smell, but it's so much better dealt with a question-comment, e.g. "did you try?" vs "this looks wrong".
I generally aim to make every comment a question ("can we..."), and not make assumptions that the author didn't think of something already. Unless I just stating an objective fact that is clearly unknown e.g. "this duplicates library function x...".
But that's not exactly what I meant with question-comments, I sometimes genuinely don't know why are they doing something like this, or have to ask for additional testing/evaluation/analysis/profiling results before I can decide that it's most likely not going to blow anything up. And as long as there's low probability of damage (or rather low expected value of damage) I am happy with the change :)
What really helps is a style guide. It shuts most of those pointless style nitpicks, God I hate them.
It worked out fine I think and, mind you, it used to be a lot harder to push out a fix. A lot harder.
The code review was preloaded.
It’s better when code reviews are suggestions, the reviewer doesn’t nitpick (or at least fixes that stuff themselves), and small things like formatting are enforced by a linter.
I haven’t asked them. I should ask for more feedback.
I haven’t asked because:
I was a latecomer to the team and they had an established process
Cultural differences between myself and them
Honestly, I’m not sure all of them are experienced enough to give a useful critique
I used to do that when I started working on open-source as well, doing code-reviews to learn the code base (often I would not submit the review to not create noise)
I wouldn't let this be a stumbling block to asking for more review. I think it's like writing an article or paper - once you've invested a lot of time into the thing it gets harder to see the warts. Having another set of eyes can really help catch that stuff, even if they aren't at the same level of expertise as you. Back in my research days the department I was in had a person who's job was basically reading research papers and providing feedback - in this case she usually focused on the paper structure, clarity of phrasing, and of course grammar - she was an expert in writing not in CS. Yet looking back a drafts of papers before and after she had given input shows a massive improvement, even if the technical content was unchanged. Similarly a smart intern can be a great source of input for making my code clearer and better, even if the algorithm doesn't fundamentally change.
Also, if like me you are prone to falling into ego traps, inexperienced folks can bypass the ego's defenses. A simple "hey why did you do it $complicated_way instead of $simple_way?" can hit a lot harder than "hey make this $simple_way" from a mentor. I think it's because I've invested time into teaching the intern and when they ask a good question like that my ego gets a boost from "I taught that kid well" to balance out the "OMG YOU THINK I'M TERRIBLE AT THIS LETS FIGHT" response. (It may also be tempered because I tend to view the intern's questions as genuine requests for input, so I start thinking of the explanation and realize $simple_way will in fact work just fine.)
So: is your code naked, on fire and carrying a bomb?
Likewise, if there's no comments on a pull request, it hasn't been read...