11
\$\begingroup\$

As I came across this answer Codility's Permutation Check in C# [answer #200620], I noticed the user reviewed quite a few requirements of the challenge, before reviewing the OP's code. My questions concern general on-topicness of such reviews. This answer serves as a mere example.

Questions:

  • Is it on-topic to review a challenge's requirements, given the OP's code also gets reviewed?
  • Should a review confront the OP with the bad practices of the challenge; e.g. can we expect OP to change (to some extend) what is provided by the challenge?
\$\endgroup\$
  • 3
    \$\begingroup\$ Whilst I don't see this as a worthwhile endeavor, and so no longer comment on these things myself. I can't see how saying how to write better code would ever be off-topic on Code Review. \$\endgroup\$ – Peilonrayz Sep 24 at 10:54
20
\$\begingroup\$

Is it on-topic to review a challenge's requirements? To the extent that the requirements force bad code, Yes.

The core of every question at Code Review is "How can I make this code better?" It is not "How can I solve this challenge?" If the asker has not gotten at least that far already, their question is off-topic here.

Therefore every question at Code Review is, by definition, going above and beyond the functional requirements which originally drove the code. It makes sense for the answers to do so as well.


Should a review confront the OP with the bad practices of the challenge? If the answer's tone is confrontational, regardless of its content, it is a bad answer (and deserves downvotes (my own answers not excluded)).

Therefore, if the reviewer has nothing to offer beyond "I don't like this challenge's requirements", the community will be better off if that reviewer just leaves a comment, or ignores the question altogether.

\$\endgroup\$
4
\$\begingroup\$

I don't see a problem with challenging the questionable requirements of a programming challenge, however, I think the answer should have given priority to the OP's code rather than the silly requirements. I would have organized the answer somewhat differently with the silly requirements after the observations about the OP's code.

\$\endgroup\$
2
\$\begingroup\$

I really want to opine that it's fine. It can be educationally useful, especially for students who might assume these code practice sites are modelling good practice.

Nevertheless, it probably is not a strictly valid target because of the authorship requirement. The challenge defines an interface. Interfaces are code. If you're hobbled by a shoddy interface you didn't write, that's third party code. It would similarly be off topic and unhelpful to say to someone invoking libpng "you've got a call to png_create_read_struct_2() and that's an unhelpfully vague function name. Perhaps it should be renamed png_create_read_struct_unmanaged()." So, it certainly shouldn't be the focus of the review, because that would make it primarily a review of third party code.

But still, it's not going to run into the actual reasons behind the authorship issue. No-one at Codility is going to be distraught about the wasted time agonizing over whether to call a variable A. No-one is going to be fired over criticism about it. And if the response is tweaked slightly to offer tips (e.g. how to use a facade pattern) about still writing good code around other people's less ideal interfaces, that's back to a review about writing good code.

\$\endgroup\$
  • \$\begingroup\$ That's an interesting point you make in the first paragraph. Addressing the lack of good practice by such well known sites is an aspect worth mentioning beginners and students. \$\endgroup\$ – dfhwze Oct 6 at 19:14
0
\$\begingroup\$

I understand the point the answerer was trying to make, but I feel like challenging the challenge's requirements has one big flaw :

The OP cannot use the review to make the code better.

The challenge has a very specific context and the question is set in that scope, it's very possible that the OP is already aware that the challenge's requirements are weird, but it changes nothing to the fact that it's the scope that needs to be followed. It's kind of like if the OP stated, in a JavaScript post : "I can't use jQuery for X reason" and that an answer proposed a jQuery answer. It's maybe better, but it doesn't help anyone.

So, in my opinion, it's of no use to the OP, which is kind of the point of CodeReview, so it shouldn't be done.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the swift answer. I can understand the case you are addressing that it would be an obvious violation of the challenge's purpose. But do you see any room for nuance? For instance, regarding code conventions, staying within the spirit of the challenge. Or would that be just trivial shenanigans? \$\endgroup\$ – dfhwze Sep 23 at 20:07
  • 2
    \$\begingroup\$ Yeah I think there's room for nuance! I think that challenging the requirements isn't a bad thing in itself, it can bring valuable insight to the OP. But an answer shouldn't only be a reqs challenge as the one you posted, because then there's no value. It should/could be an addendum to an actual review, IMO. \$\endgroup\$ – IEatBagels Sep 23 at 20:10
  • 6
    \$\begingroup\$ "it's very possible that the OP is already aware that the challenge's requirements are weird" - it's also very possible that the OP is not aware about the fact that the challenge's requirements are weird, in which case it's good to point them out. Even if it is not applicable to the challenge itself, at least the OP has learned to spot some bad practices. \$\endgroup\$ – Simon Forsberg Sep 25 at 7:00
  • 7
    \$\begingroup\$ The OP can absolutely use the review to make the code better - but perhaps not within the scope of the specific challenge. \$\endgroup\$ – Simon Forsberg Sep 25 at 7:01
  • 1
    \$\begingroup\$ Don't forget that Stack Exchange exists to help not only the OP, but the whole community as well. Such reviews can benefit others. \$\endgroup\$ – L. F. Sep 30 at 10:10

You must log in to answer this question.

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