16
\$\begingroup\$

I want to do something that is a little unusual, but not obviously off-topic. As a result, I thought I'd post here first.

I got a pull request into a code base I (professionally) maintain. I'd like to post it here. I didn't write the code, but it is code that I maintain, so this is on-topic. That being said, there are reasons I feel it might be off-topic. Namely:

  1. I obviously looked over the code myself. By my standards the code is (to put it mildly) very poorly written, and will be rejected.
  2. As a result, I'm not really looking for suggestions on how to improve the code: I know what is wrong with it, and it will be completely rewritten.
  3. Rather what I'm interested in is how other (experienced) developers would view the code, and how they might approach the same problem. As a result I'm now realizing that, in a very real sense, I'm still looking for a code review.
  4. For various reasons, I'm also thinking about adding this code sample to my list of interview questions for candidates (we hire on a fairly regular basis). It has enough rookie mistakes in it that I feel like I should be able to present it to an experienced developer (with appropriate background context), ask "What is wrong with this code?", and get some coherent answers.

In short, I'm looking for a code review, but not for the usual reasons. While my reason for asking for a code review probably shouldn't matter (as long as it is on-topic), I feel like part of my motivations for asking for the code review is not in the spirit of this site (i.e. reason #4 above). As a result, I'd like some feedback on whether or not I might be on topic.

\$\endgroup\$
20
\$\begingroup\$

Thank you for your transparency.

If you had written the PR (or merged it), it would be on-topic.

You already know you'll be rejecting the PR - putting it up for review here would boil down to picking on someone else's code, which is one of the reasons why reviewing other people's code is off-topic: we're not The Daily WTF!

As for #4, that's indeed what we do ("does this code make my ass look fat?"), but we require that it's your code, not someone else's. In this case not for licensing issues (you do own the code base the author intends to merge their work into), but because reviewing code isn't a bashing session. ...even if code-bashing can be fun, when it's done at the expense of someone that didn't ask for it, it's just not right.

You can nudge the contributor to post here though. We've got plenty of questions asked by people that are posting the code they submitted in an interview and didn't get the job. I don't see how that's different from posting code you submitted for a PR that was rejected.

Something like:

Hey,

Unfortunately this isn't going to work out. There's too much work that needs to be done on this PR to turn it into something that would be acceptable in our code base.

Feel free to post your code up on Code Review Stack Exchange (describe its purpose, give plenty of context, present it to the reviewers), there's a lot of people there that can help you see what's wrong with this code, and how to fix it.

Good luck!


Note that the "rule" is indeed easy to circumvent - hence thank you for your transparency at the beginning of this answer.

Sure one could game the system and word the post in such a way that it's not quite possible for anyone to know/guess that it's not your code you're asking feedback about.

But nobody likes dealing with DMCA takedowns and whatever may or may not legally apply - we just want to help people that genuinely want to get their work reviewed, in good faith.

So this isn't about "playing by the rules" - it's about basic common sense and decency; it's about adhering to the rules this community has agreed upon. Not because we tell you to, but simply because it's the right thing.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. Obviously, that makes a lot of sense: I had my own doubts, which is why I posted this here first. I have to be clear about one part though: it certainly isn't my goal to pick on the person in question. Rather, it's always good to double check my own thought processes. I'm confident in my conclusions so I don't think the overall answer is going to change (i.e. this code is no good), but I always appreciate a second pair of eyes: sometimes you learn stuff anyway, even when you've done it a hundred times. \$\endgroup\$ – Conor Mancone Sep 20 '17 at 17:49
  • \$\begingroup\$ Although now you've made me curious: it was my understanding that code is on topic if it is your code or code you maintain: from that perspective, I would have expected something like this to be generally on-topic. Am I confused? \$\endgroup\$ – Conor Mancone Sep 20 '17 at 17:51
  • \$\begingroup\$ The spirit of the rule is that we want reviewees to post their code. Take the latest rubberduck question for example: I own the repository and maintain the project, but the person working on a refactoring feature is putting their code up for review here well before they submit the PR. As the owner of Rubberduck, that code currently belongs to their fork, not mine; it's their work, not mine. If they want to post it here, it's their decision. After it's merged though, then it's part of my code base, and I'm the one that now has to maintain it. Subtly different, but important IMO. \$\endgroup\$ – Mathieu Guindon Sep 20 '17 at 17:58
  • \$\begingroup\$ I have to downvote this because your answer make it sound like it's off-topic (as indicated by the above comments as well). I don't think it is off-topic. The OP of this meta question is the maintainer of the code. I would however recommend against posting it here and I would possibly downvote the question for being posted for the reasons stated here on meta ("I got this PR and I reviewed the code myself so I know it is not good code"), but I do think it is on-topic. \$\endgroup\$ – Simon Forsberg Sep 20 '17 at 18:24
  • 7
    \$\begingroup\$ @SimonForsberg the OP doesn't maintain the code that's in the PR - yet. It's not merged, it's not in his repository, it's not his work, and he might not even have write access to that fork. I'm 100% sure we've closed questions on these grounds before, and will continue to do so in the future. I firmly disagree with it being ok to take someone's work and put it up for review on this site - it boils down to "hey folks look at this mess of a PR I got, whadaya think?" \$\endgroup\$ – Mathieu Guindon Sep 20 '17 at 18:28
  • \$\begingroup\$ Well everyone agrees that it is off topic, so that's something :) I think a big part of why I wanted to do this is to see how others assess the same bit of code. Again, it isn't an attempt to pick at the person (or their code), but I'm newer to hiring than to code, and a big part of hiring is assessing how well other people write code. It's one thing to say "This is bad code" but it is another thing to say "This is bad code and a person applying for X job should know better". Seeing how other people view the code was one idea I had on how to start calibrating that sense better for myself. \$\endgroup\$ – Conor Mancone Sep 20 '17 at 18:46
  • \$\begingroup\$ @Mat'sMug Okay, yes, the fact that it is not merged is indeed the deal-breaker. I agree with you, but I think you should edit your answer to make it clearer that if the PR would be merged, then it would be on-topic. \$\endgroup\$ – Simon Forsberg Sep 20 '17 at 19:32
6
\$\begingroup\$

As Mat's Mug has explained, the proposed question is not within the scope of Code Review.

However, you say you already know what's wrong with the code. So why don't you fix it up first and ask for a review afterwards? You maintain the code, you turned something ugly into something shiny, we'll help make it even more shiny.

Basically, it's perfectly fine you're looking for a review. You're just too early in the process.

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

It sounds like you have the legal right to post it here, so I believe it should be OK. You are less interested in how to improve the code than you are in getting a feel for how others would improve the code, but I don't think that disqualifies it.

I think rejecting this simply because you haven't accepted the PR is just asking for someone to game the system: you accept it, roll it back and now it's part of the code you maintain. As long it doesn't have any known bugs, you might even accept it while working on a replacement.

Rejecting your CR because the original author didn't ask for it is again, IMO, just asking for someone to game the system -- if it's ok because it's part of the code base you maintain, even though you didn't write it, then make it part of that code base and it's ok.

Finally, I disagree that it is not part of your code base -- it is not an ACTIVE part of your code, in that it doesn't change the compiled output, but the same thing could be said for comments, or older versions of the code. It's code you are working on, even if that work is just declining to pull it into the main branch.

\$\endgroup\$
  • 1
    \$\begingroup\$ There's no point in gaming the system, it's a waste of everyone's time. OP knows what to improve and where and how; there's nothing to be gained for the OP until they merge the PR and proceed themselves with the changes they want to make. The code you put up for peer review should be as good as you can make it, otherwise you're not getting the best value out of this site. Want to game the system? Not a good idea. There's DMCA takedown procedure, suspensions, and all kinds of nasties awaiting down that road. \$\endgroup\$ – Mathieu Guindon Oct 2 '17 at 11:56
  • \$\begingroup\$ @Mat's Mug: He has the legal right to post it, DMCA doesn't apply. Another way of saying "game the system" is "follow pointless rules". You want it to be part of the main branch of his code, then he can check it in, ask his question, then rollback the checkin. How is that not 100% in line with the proposed rule? It requires a useless checkin and rollback, but hey, at least CR users won't complain about him posting code WHICH HE HAS EVERY LEGAL RIGHT TO POST. \$\endgroup\$ – jmoreno Oct 2 '17 at 12:06
  • 1
    \$\begingroup\$ And exactly what is the point of doing that, beyond wasting everyone's time? \$\endgroup\$ – Mathieu Guindon Oct 2 '17 at 12:08
  • \$\begingroup\$ @Mat'sMug: as for him not getting the best value, horse pucky. The value of a code review is always to have another viewpoint of the code, he gets that. \$\endgroup\$ – jmoreno Oct 2 '17 at 12:09
  • 1
    \$\begingroup\$ This community asks that people post in good faith code that they wrote themselves. Sure you can be a jerk and game the system. But the rule isn't stupid or pointless. It's something we've all agreed was basic common sense. \$\endgroup\$ – Mathieu Guindon Oct 2 '17 at 12:11
  • 2
    \$\begingroup\$ @Mat'sMug: He might get that now, he wouldn't have gotten that if he'd done the above without first telling us everything. Even now, the accepted (and upvoted) answer says "or merged it". So merge it, say you've merged it and then don't say you've deleted it -- which since it happens after the question, isn't really relevant anyway. \$\endgroup\$ – jmoreno Oct 2 '17 at 12:12
  • \$\begingroup\$ @Mat'sMug: the contributor is told that the OP now maintains that code, and is entitled to post it here to be bashed. Or the rules are changed and only code that the poster wrote is acceptable. Either it's acceptable to post someone else's code because you have a legal right to do so, or it's not. \$\endgroup\$ – jmoreno Oct 2 '17 at 12:16
  • 4
    \$\begingroup\$ This isn't about legal rights, it's about basic common sense and decency. I wouldn't want to contribute to a repository with an owner that somehow becomes known for doing that. \$\endgroup\$ – Mathieu Guindon Oct 2 '17 at 12:39
  • 1
    \$\begingroup\$ @Mat'sMug Is a DMCA a likely event if the code has been merged? And how likely is it to be approved? It seems very unlikely to me. It's not only about legal rights, but legal rights is a part of it. \$\endgroup\$ – Simon Forsberg Oct 2 '17 at 20:00
  • 2
    \$\begingroup\$ Even if you did merge the code in GitHub, if you posted a Code Review question that said "Hey, I just merged some crap code that someone else wrote; can you help clean it up?" — I would still consider it an off-topic request to review someone else's code. As the maintainer of the codebase and the poster of the question, you should be prepared to defend how the code is written, as if it were your own best work. \$\endgroup\$ – 200_success Oct 3 '17 at 18:04

You must log in to answer this question.

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