19
\$\begingroup\$

Our Help Center says:

Do I want feedback about any or all facets of the code?

However, sometimes we might have some code that we are aware has flaws in it, or we would just for one reason or another want reviewers to not comment about something.

When asking a question on Code Review, can we ask reviewers to not focus on something?

This question was inspired by, and meant to be a more generalized version of Asking questions - including full code

\$\endgroup\$
16
\$\begingroup\$

Yes. It is fine to ask reviewers to preferably avoid commenting on something.

Of course the reviewers can comment about it anyway, and then it will be up to the community - and the original poster - to determine if the review is helpful or not.

If you're not interested in comments about naming, indentation and styling conventions, a hash function, or anything else, then it is good to explicitly say: "I am aware about xyz but (fill in a reason for why xyz is bad in the code you have included)"

However, if your code really is horribly wrong and you don't have a good reason for why it is horribly wrong, you will most likely hear it anyway.

An example: You don't want suggestions about using prepared statements? Sure, you can ask reviewers to not suggest that, but they will probably suggest it anyway

\$\endgroup\$
  • \$\begingroup\$ Generally speaking, there are some instances where business logic defines why code might be ugly in one particular area. ("I know I use a lot of lists, but this is required for one of the application features.") In my opinion, if you can justify the reason for it and it makes sense, I think it's perfectly valid to expect that reviewers would honor that request (or at least comment on more than just that). \$\endgroup\$ – 410_Gone Dec 26 '16 at 20:44
13
\$\begingroup\$

That is a trickier idea than what it sounds like.

If I don't say anything, answers can cover anything.

If I say I have a performance concern, answers can still cover anything, and I can accept the one that best addresses performance.

Now, if I say I have a performance concern and I don't want to hear about anything other than performance... then I'm not asking for a code review, I'm asking for a performance tune-up.

A recent answer was downvoted for not addressing a performance issue, while still actively reviewing the OP's code. The (assumed-to-be) downvoter left a comment:

The OP didn't ask for a critique of syntax, but instead was looking for a way to make the code run with better performance. Yes, this is a code review site, but focus on the question at hand.

I feel that asking reviewers to not look at X or Y would go against the spirit of this site ("do I want feedback on any facets of the code?"), and could reduce the number of answers given. We don't need that.

\$\endgroup\$
  • \$\begingroup\$ As I stated in my answer, reviewers still can comment about xyz if they really feel that there is something blatantly wrong with xyz. Sure, it could reduce the number of answers given, but helping the OP with his or her questions is much more important than the Area51 stats. \$\endgroup\$ – Simon Forsberg May 14 '14 at 0:03
  • 5
    \$\begingroup\$ @rolfl's answer on the linked question says it all, I find. I you want people to focus on performance, give us code that only gives us performance to focus on; if your naming is misleading and your casing is annoying, you should expect to be told. If you know the naming is misleading and the casing is annoying, and you still post that code up for review, then the best way to prevent answers that focus on these minor issues, is to post an answer that addresses them, yourself. \$\endgroup\$ – Mathieu Guindon May 14 '14 at 0:17
  • 6
    \$\begingroup\$ ^^^^ after doing that, you may as well fix the code before you post it.... so only the performance problem is 'visible' \$\endgroup\$ – rolfl May 14 '14 at 0:19
  • \$\begingroup\$ @rolfl I was getting to that ;) \$\endgroup\$ – Mathieu Guindon May 14 '14 at 0:19
10
\$\begingroup\$

Preemptively acknowledging a known weakness in your code is fine. That lets reviewers know that you are working on improving that aspect of the code, and could help redirect attention in a more productive direction. You might even have a good reason why the code has to suck — for example, if you are integrating with a badly designed API — in which case the explanation would be helpful.

However, instructing reviewers to ignore a particular kind of badness in your code violates the rules and the spirit of the site. There may be reasons why reviewers might decide to pick on something — for example, the performance or security problem that you want to ignore might be even worse than you had imagined.

\$\endgroup\$
  • 3
    \$\begingroup\$ If there's two things that askers is very unlikely to instruct reviewers to not care about, it's performance and security. It more tends to be about things like variable names and code style. \$\endgroup\$ – Simon Forsberg Sep 7 '14 at 12:03
1
\$\begingroup\$

While asking the reviewer to limit their answer to a specific concern may be possible, it is probably a better(not to mention more polite) idea to ask specific questions to get a specific answer. Instead or trying to limit the reviewer to what you want them to review, Ask them to review a specific concern.

If the reviewer answers your question and then chooses to also review other aspects of your code, then that is of no concern to you. Just read the part that is relevant to what you wanted, and move on. Frankly though, I appreciate it when people review my overall code, rather then just focusing on a specific part.

And we must remember, no one on code view has to answer your question at all. It's the reviewer's choice. I don't think that limiting people to what they can and cannot review will help your chances of getting a review.

\$\endgroup\$
0
\$\begingroup\$

As a newer user of CR, and seeing some of the questions regarding user retention, I'll throw my opinion out there as well.

The help center touches on this question where you pointed out, but not with enough clarity to make newer posters entirely aware of what is and isn't the best practice - this could frustrate users and prevent them from wanting to return if their experience is sub-par.

I would suggest perhaps an update to the help center making this clear, or even a snippet in the tour with @Simon's suggestion of including a disclaimer, should the OP's question be dumbed down for the sake of posting.

If disclaimers were used and respected, more to-the-point questions could be asked, with functioning code centered on the simplest base case scenario.

\$\endgroup\$
  • 2
    \$\begingroup\$ Even if this were added to the Help Center (although not every new user reads it), these differences from other SE sites make it harder for the community to moderate the site. Every avid user must know all of these non-typical site policies, otherwise improper site usage can occur. As we are currently in beta, we can still work through these obstacles. \$\endgroup\$ – Jamal May 14 '14 at 1:59

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .