12
\$\begingroup\$

I'm looking at Java refactoring - reduce cognitive complexity by extracting private methods (note: the question has been edited since I wrote this. At that time, I was looking at Revision 4) and in response to my comment that it's lacking description of the code's purpose, the author replied:

I'm not asking to review the code, I'm asking to review the refactoring. That means a review of a review. A meta-review. The refactoring intends to achieve lower complexity, better readability, better testability, better maintainability.

Is this question on-topic? Can we review this new implementation with only the original code as explanation? Or does the new code really sit for review in the usual way, with the old code merely present as illustration of an earlier attempt?

Although the specific question has now been edited, it may be worth considering what the requirements are for a changeset to be reviewable, as it's surely not the last question of this type.

\$\endgroup\$
9
  • 2
    \$\begingroup\$ The author has removed that comment and has complied with the requests made to improve the question. \$\endgroup\$
    – Amedee Van Gasse
    Sep 17, 2019 at 13:08
  • \$\begingroup\$ @AmedeeVanGasse: yes, I've seen that now, and edited this question accordingly. It's still useful to know how to apply our criteria to similar questions. \$\endgroup\$
    – Toby Speight
    Sep 17, 2019 at 13:10
  • \$\begingroup\$ Was the original code posted previously as a question? \$\endgroup\$
    – pacmaninbw
    Sep 17, 2019 at 13:21
  • \$\begingroup\$ @pacmaninbw, not as far as I could determine (the description says/said, "This is code, written by our dev team, ... I've had a go at reducing the complexity and would love some feedback.") \$\endgroup\$
    – Toby Speight
    Sep 17, 2019 at 13:24
  • 2
    \$\begingroup\$ A changeset itself is not reviewable. 2 sets of code with a changeset, sure, why not. \$\endgroup\$
    – Mast Mod
    Sep 17, 2019 at 13:56
  • \$\begingroup\$ @Mast, do you believe that a plain-text description of purpose is also required? Or that the old-code is itself sufficient description? \$\endgroup\$
    – Toby Speight
    Sep 17, 2019 at 14:28
  • 3
    \$\begingroup\$ A description of purpose is always required in my opinion. No exceptions. \$\endgroup\$
    – Mast Mod
    Sep 17, 2019 at 16:49
  • \$\begingroup\$ @Mast the question currently includes the old code, the new code, the diff, and a plaintext description of purpose. Is that enough or is more information needed? \$\endgroup\$
    – Amedee Van Gasse
    Sep 18, 2019 at 14:16
  • \$\begingroup\$ @AmedeeVanGasse It's enough. More information on what it's doing would score bonus points with me, but it's enough. \$\endgroup\$
    – Mast Mod
    Sep 18, 2019 at 19:53

2 Answers 2

Reset to default
14
\$\begingroup\$

I'm not asking to review the code

This alone dismisses the idea of a meta-review as invalid, for me. We only review code. The OP could easily just post the new code and receive valuable feedback about it. Whether that's feedback about portions of the new code that have been affected by the changeset or portions of the new code that were unaffected is fairly immaterial.

One of our express policies is that any insightful observation about a post's code is on-topic, whether it match the OP's stated review goals or not. Constraining the question to say "don't review this old code - it's not applicable; only review the diff between the old and new code" confuses the matter. Among other things, it hinders question searchability - what if someone searches for a particular code construct and finds a match only in the old code, and there's by design no commentary on it at all?

\$\endgroup\$
6
  • 7
    \$\begingroup\$ Good point. First rule of Code Review is wanting a review. What else do you expect from a review site after all? \$\endgroup\$
    – Mast Mod
    Sep 17, 2019 at 16:50
  • \$\begingroup\$ This answer is no longer valid with the current version of the question. The OP has complied with all requests in comments. \$\endgroup\$
    – Amedee Van Gasse
    Sep 18, 2019 at 14:20
  • 5
    \$\begingroup\$ @AmedeeVanGasse Re-read the last part of the meta-question: Although the specific question has now been edited, it may be worth considering what the requirements are for a changeset to be reviewable, as it's surely not the last question of this type. This is no longer about the current version of the question, but about the idea of a strict meta-review. \$\endgroup\$
    – Reinderien
    Sep 18, 2019 at 14:23
  • \$\begingroup\$ Are saying that the OP, a first time poster, did not immediately comply with the questions, requests and suggestions to improve the question, thereby warranting a discussion on meta? \$\endgroup\$
    – Amedee Van Gasse
    Sep 18, 2019 at 14:25
  • 6
    \$\begingroup\$ Whether the OP is first-time or a veteran with 10k rep doesn't matter, and the "speed of compliance" doesn't matter either. If we see something that doesn't have a clear entry in the policy documentation, it's worth clarifying in meta. \$\endgroup\$
    – Reinderien
    Sep 18, 2019 at 14:28
  • \$\begingroup\$ @AmedeeVanGasse At first, this meta was about your specific question. But after a small comments exchange OP decided to make this about general site policy instead, using an older revision of your question as a mere example. \$\endgroup\$
    – dfhwze
    Sep 18, 2019 at 14:50
6
\$\begingroup\$

We have a tag . I believe this indicates that we do support these kinds of reviews. The user went one step further by including the git diff; I don't see that as a problem.

\$\endgroup\$
2
  • 4
    \$\begingroup\$ AIUI, a comparative review still requires an explanation of what the code is for. The question at the time had no such explanation, so wasn't on-topic as a comparative review (though it would be now, after editing added the description). Am I right? \$\endgroup\$
    – Toby Speight
    Sep 17, 2019 at 14:26
  • 1
    \$\begingroup\$ @TobySpeight I didn't look at the history of the edits when I wrote my answer. The title changed and that is what gives the explanation of the code, so you are correct. \$\endgroup\$
    – pacmaninbw
    Sep 17, 2019 at 22:28

You must log in to answer this question.

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