3 ms·

s/teach/be petty/. Nobody cares about a method name and if I care, I can send my own cleanup change later instead of holding up work of others. A good compromis

s/teach/be petty/. Nobody cares about a method name and if I care, I can send my own cleanup change later instead of holding up work of others. A good compromise is to approve and still suggest a different name in comment and then trust competence of my coworkers to decide for themselves. On the other hand, if someone is loading images on a main thread of a UI app, teaching is a primary priority compared to getting the change merged. Obviously they are misunderstanding big parts of platform they are developing for and, until I get them up to speed, they will have a hard time being productive. So even if it takes an extra week, it's important that they learn the best practices and how to apply these in specific cases. I am absolutely against mind games like asking vague questions and holding up work until the author gives me my preferred answer, neither of us have time for that.

amatecha 4y ago
Yeah, totally agreed. My general principle on method names is: suggest an alternative, but it's a purely-optional suggestion. Sometimes I can think of a far more effective name for something, and I'll suggest it, but say "not required, just suggesting". There are a ton of things in code reviews that could be improvements, but are also not a big deal, or quite subjective. Then there are the things that seriously affect code quality and future maintainability, which would be the kind of thing that should hold up the review until improved or corrected. In either case, being completely clear and straightforward is always a solid approach. Asking weird rhetorical questions (as opposed to clear and direct questions) does not help the process and generally elicits uncertainty and self-doubt in the code submitter.
tyingq 4y ago
Considering context is helpful also. Like perhaps they implemented some function in an odd way because other similar functions in the existing library are also that way.
- nonethewiser 4y ago
  That's why _honestly_ asking why they did something a certain way first can be a good idea.
britch 4y ago
I agree approve with comment and trust is the way to go. I will say if I submit a change and my reviewer immediately opens a change to make minor edits I'm going to be annoyed. Either it's important enough to bring up in review, or it's too minor to bother with.
nonethewiser 4y ago
I think the example is too contrived. Like you said, make a non blocking comment about the name and be done. For less trivial examples, oftentimes I think honestly asking "why did you do it this way?" Before making a suggestion is a good idea. Often we dont have the same context nor know the intention. Emphasis on "honestly." If you just want them to do something differently then suggest that directly.
lkbm 4y ago
A misleading method name can cause serious screw ups down the line. At best, they make reading through the code later much more confusing. It's not petty to ask for accurate and comprehensible method names. The problem here is the mind games. If you think a method name is a problem, just say so. Don't hint and waste people's time and energy. Just say what you mean. (If you can't do that, you're in a terribly unhealthy work environment, and fixing that should be a top priority.)

amatecha 4y ago

Yeah, totally agreed. My general principle on method names is: suggest an alternative, but it's a purely-optional suggestion. Sometimes I can think of a far more effective name for something, and I'll suggest it, but say "not required, just suggesting". There are a ton of things in code reviews that could be improvements, but are also not a big deal, or quite subjective. Then there are the things that seriously affect code quality and future maintainability, which would be the kind of thing that should hold up the review until improved or corrected. In either case, being completely clear and straightforward is always a solid approach. Asking weird rhetorical questions (as opposed to clear and direct questions) does not help the process and generally elicits uncertainty and self-doubt in the code submitter.

tyingq 4y ago

Considering context is helpful also. Like perhaps they implemented some function in an odd way because other similar functions in the existing library are also that way.

nonethewiser 4y ago
That's why _honestly_ asking why they did something a certain way first can be a good idea.

britch 4y ago

I agree approve with comment and trust is the way to go. I will say if I submit a change and my reviewer immediately opens a change to make minor edits I'm going to be annoyed. Either it's important enough to bring up in review, or it's too minor to bother with.

nonethewiser 4y ago

I think the example is too contrived. Like you said, make a non blocking comment about the name and be done. For less trivial examples, oftentimes I think honestly asking "why did you do it this way?" Before making a suggestion is a good idea. Often we dont have the same context nor know the intention. Emphasis on "honestly." If you just want them to do something differently then suggest that directly.

lkbm 4y ago

A misleading method name can cause serious screw ups down the line. At best, they make reading through the code later much more confusing. It's not petty to ask for accurate and comprehensible method names. The problem here is the mind games. If you think a method name is a problem, just say so. Don't hint and waste people's time and energy. Just say what you mean. (If you can't do that, you're in a terribly unhealthy work environment, and fixing that should be a top priority.)