Asking about code in a pull request to my code base

Question

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.

Answer 1

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.

Answer 2

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.

Answer 3

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.