11
\$\begingroup\$

In my first CR question, I was looking for feedback about my implementation of a graph data structure and an algorithm for depth first traversal, as I am studying these at the moment.

Although I appreciate many of the reviewers' comments, there seemed to be a focus on code that I deliberately put together quickly for the purpose of asking for review, as I would do on SO.

My question is this: when asking for review, should full code be included, or should we work code down to something simplified so the meat of what we are asking for can be looked at more thoroughly?

In my linked question, for example, I used a very basic hashing function that got torn to bits in the reviews (rightfully so); but my main question was regarding the implementation of the graph structure - should I have included a lengthier HashTable class, even though that wasn't what I was looking for review on?

\$\endgroup\$
15
\$\begingroup\$

The code that you want to have reviewed should be included.

There are several questions on Code Review containing a whole lot of code, I myself tend to have very much code in my questions, and it's OK to have a lot of code in your question. The only limit is the limit built into the StackExchange system, 65k characters per post. (Be aware though that having more code will likely cause answers to take longer time, even though an answer doesn't need to point out "everything")

If you're already aware that parts of your code has flaws and that you'd prefer the reviewers to not focus on them, say that in your question. If you don't explicitly say anything about it, then reviewers can comment on anything in your code. Of course, they can still comment on the things that you don't want them to comment on, but I don't think most people will.

For example say: "I am aware that this hash function is horrible, I am just using it for a test. In production I will use something much better which I might put up for review later."

\$\endgroup\$
  • 2
    \$\begingroup\$ The idea of the disclaimer will be helpful in the future, as I'm sure I'll more than likely continue to weed out extraneous code and simplify it so as to assist reviewers to focus on what I'm truly concerned about. \$\endgroup\$ – tloveless May 13 '14 at 23:00
21
\$\begingroup\$

I have my 'pet theory' about Code Review. This is my personal theory, and it is about human psychology, so you will have to take it with some salt..... It goes like this:

  1. You post code for review, and...
  2. noone knows how good you are as a programmer
  3. they do one of two things:
    • find the most glaring problems in your code, and identify what they think will make the biggest improvement to your code.
    • find the most simple things to review for a quick answer (indentation, namespaces, etc.)
  4. they focus on that in their review
  5. you learn the most, get the best value from the review

The problem with this, is that you maybe know the parts that are the worst things in your code, and you don't want to address those (yet, or you have a different plan for it, or something).

If you post sloppy code, you get a review of the sloppiness.

If you post code with glaring problems, you get focus on the glaring issues.

Now, if you want to focus reviewers' attention on some subtle interaction, or some multi-threaded complexity, or other sensitive issue, you have to post code that is neat, focused, and mostly issue-free.

The better the code is that you present, the more capable the reviewer will think you are, and the deeper insight they will offer in to your (subtle) issues.

In your case, I strongly recommend you fix the 'glaring' issues of the hash function, and then resubmit your code as a new question, and you can then hope for a review that takes it 'to the next level'.

This has a strong precedent in code review, and is well received.

See: How to post a follow-up question?

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

In case you're aware of this guideline from the Help Center:

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

Anything you include, provided that it was written by you, is subject to any type of review. You can include whatever you'd like, which doesn't necessarily have to be the full code. However, it must still be enough code to where it can be considered working if someone were to try to test it.

Our code length expectation differs from SO in that we're not specifically looking for simplified code-- debugging is not involved. If you feel that you would benefit from including the full code (provided that it fits into the post), then you may do so. You can still ask specific questions about the full code and do not have to simply the code just to ease this.

\$\endgroup\$
  • 1
    \$\begingroup\$ While I agree on most things on this answer, there's one thing I think is missing from it: The possibility to say "I am already aware of xyz so please don't comment about that". See my answer. \$\endgroup\$ – Simon Forsberg May 13 '14 at 22:56
  • 1
    \$\begingroup\$ @SimonAndréForsberg: I'm already aware of that. I haven't come across it too much, so I didn't think of mentioning it. \$\endgroup\$ – Jamal May 13 '14 at 22:58

You must log in to answer this question.

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