2

I think that code reviews are great and very helpful for everyone. With that being said, from time to time I've received feedback on a pull request (leading to the PR being delayed for at least as long as it takes for another round of responses from everyone involved, if not longer) that I think is so completely out of left field that I don't even know how to reply. For example:

  • (discussing a single-page website built with an Angular-like framework): "I think we should serve these as separate pages because otherwise users will be confused"

  • (discussing a C89/C99/C11 stdlib function): "I think we should avoid this for the sake of portability and re-implement it ourselves"

  • (discussing functionality that we need a compiler extension for): "Although we use this in multiple other places, I think we should re-implement it ourselves here for the sake of portability"

Although I don't want to establish a negative tone during code reviews, I find it very hard not to get frustrated with commentary like this, especially when the reviewer is very insistent about it (i.e. multiple rounds of debate back and forth). When I see commentary like this, I strongly suspect that the reviewer is just looking for an excuse to talk, and that he or she would find some reason to semi-randomly permute pretty much any pull request submitted. I feel like this subverts the point of the code review process and drags down the productivity of the team as a whole.

I don't know how to respond to these kinds of questions without coming off negatively, because I find it hard to believe that they are being asked in good faith (e.g. if you genuinely worry that functionality guaranteed by your language's spec isn't portable enough to use, how do you get anything done at all?). How can I reply to this sort of commentary effectively?

closed as primarily opinion-based by user22815, Robert Harvey, Jörg W Mittag, user40980, gnat Mar 8 '16 at 7:10

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
  • 2
    This is not about explaining something random to someone random. This is about the software development process, and the distinction and place of code reviews within it, say as compared to scrum meetings, design reviews, or other aspects of the software development process. It is as "on topic" to programming as questions about git flow and git hub, which are commonly supported on this website. – Erik Eidt Mar 8 '16 at 1:15
  • 1
    For the second 2 points; just say "We can write a more portable version ourselves later if/when it's ever actually needed". ;-) – Brendan Mar 8 '16 at 2:47
  • Put bluntly it helps if those reviewing the code know what they're talking about rather than trying to cross-apply irrelevant experience or just raising something for the sake of it. Make it crystal clear that any modifications have a development cost and ensure that every change is given a priority and that the results of the code review are recorded along with a full justification for each change (by the reviewers). If any are just noise, raise these with a 3rd party as a sense check before starting development. – Robbie Dee Mar 8 '16 at 12:34
4

You need to position code reviews within your development process. Code reviews should probably not be the sole place for feedback, as obviously some feedback should have been voiced/discussed and worked out much earlier. Still other feedback should spawn its own thread of work without necessarily impacting with the current work item under review.

The first bullet point probably ought to be off-topic to the code review, and instead be on-topic during your customer meetings, scrums, or discussions of user stories. In other words, it is an issue of the design of customer interaction; as such it should have been raised earlier, and it should have been raised by someone more representing the client/customer rather than fellow programmers (who are reviewing the actual code).

The last two bullet points probably ought to spawn their own refactoring thread(s), but not necessarily hold up the code in question, in particular, when the code in question is not the sole "offender". It is generally considered best practice to do multiple small check-ins rather than large ones, so significantly expanding the scope of some code change in order to incorporate a broad refactoring (requiring that you fix code outside of the scope of one check-in) is conflating matters, i.e. a step in the wrong direction. It is also best practice to keep refactoring and feature change separate whenever possible, as separate check-ins, thus, this also suggests that adding a refactoring into the feature check-in (or separate refactoring check-in) is a conflation of purposes.

Not the answer you're looking for? Browse other questions tagged or ask your own question.