4 ms·

Yeah, I've been asked to make an implementation worse before because it didn't look like the reviewer's code. I used a static pure function to implement some ba

Yeah, I've been asked to make an implementation worse before because it didn't look like the reviewer's code. I used a static pure function to implement some basic functionality and made that function public so that it could be unit tested. The reviewer asked me to make the pure function impure and protected and then use the friend keyword to couple the class to the unit tests. When I asked him about it all of his responses were on the principle that we shouldn't use too many techniques from the language, and we're already using friend to do what I was doing with the static function. Needless to say since this was a favor for the reviewer and his feedback was basically "completely reimplement the solution for a totally arbitrary reason," the pull request languishes to this day.

KKKKkkkk1 9y ago
It's nice to have the luxury to have a PR languish because of obdurate code reviewers.
throwanem 9y ago
I mean you could just explain why you're not going to do that, and if he still refuses to sign off because of it, find another reviewer or escalate or both. I agree that it's no good having a reviewer with bad priorities hang you up over them. But you're the one whose task it is to carry through if you can, and parsing blame doesn't ship code.
matwood 9y ago
If I'm reading this right, it sounds like the reviewer was arguing for consistency with the rest of the code base. If so, I can agree with the reviewer but I would have added that if the static function was superior how much work would it be to make them all consistent.
- jschwartzi 9y ago
  It would've required completely rewriting the solution. The reason we used friend functions in the past was when we had god objects that we needed to test. It did not make any sense here since the class under test had a single responsibility. It would also have made the implementation more complicated and would have tightly coupled the unit tests to the implementation of the class.
- dahart 9y ago
  Being a programmer, it's extremely likely that @jschwartzi was already trying to be consistent in every way he knew how, and every way that mattered to him. Bringing up a random and surprise way to make something consistent in a code review, for reasons not already written down and agreed upon in your team's coding standard, should generally be deemed inappropriate. I recommend codifying the axes of consistency in your org's coding standard and style guide -- that's what they are, after all. Make sure everyone agrees. If a new consistency rule seems important after multiple similar PRs, it should go through a process where the whole team discusses whether it should be adopted into the style guide. In my experience, "consistency" is one of those abused opinion-principles. Everyone has a different idea of what it means to be consistent. Some consistency is a great idea, but in general, appeals to consistency that aren't already in the coding standard should be prioritized lower than simplicity, and lower than whether the code & tests work as designed.
  - matwood 9y ago
    The problem is that what I'm talking about is not style based. In a simple, made up case if I have a class that contains utility functions and I task someone to add a new utility function I would expect it to end up in the already existing class. Making a new utility class for the new function because the old class is too large in the new programmers opinion is breaking consistency. The class may be too large, but putting the new lone function off by itself is now an exception that everyone else will have to deal with. That one inconsistency then leads to others adding their own utility classes and then you have a big mess with some functions scattered across new utilityX, utilityY, and the old utility. As I said above, I would have handled the review differently and suggested/asked if all of the functions involved could be refactored to use the new improved method. In my example above, I would have asked if he/she could go ahead and break up the utility class as part of the change to add the new function. If the work was too much to bring it all up to date, then fit with what already exists and put a ticket in to do the explicit refactor later.
    - dahart 9y ago
      I understand and appreciate your point. And there are definitely situations where I agree with your take. Also I'm glad you'd phrase the review as a request and not a demand, that is closer to the right approach. But, as an engineering manager, I'm making a procedural point about code reviews, not disagreeing with your opinion. If the type of consistency being requested in the code review is not in the coding standard, then it's not fair game for blocking a PR or even making judgements, pure and simple. It's certainly okay to discuss it, and for the reviewer to ask questions and see if the submitter would like to improve consistency according to the reviewer, but if it's not part of the standard, then it's not a rule. "Consistency" is not a rule by itself anyway, it's a vague unspecified idea. There's no such thing as 100% consistency in multiple pieces of code, unless they're 100% copies. Every single file has unique differences that the team has to deal with. There's not much evidence that being extremely nit-picky about consistency improves safety or productivity, and in general, there's no reason in advance to think the "exception" will cause anyone else any trouble. Worrying about it may be premature optimization. To be more specific to the example above, there are times when visibility of functions should be limited, for safety reasons. Exposing functions selectively using friend declarations is generally seen as good practice in many situations, it's not a bad default choice. But, static public functions absolutely have their place too, and moreover they are a simpler language feature than friends. Simplicity is (to me) more important than consistency. In the example above, @jschwartzi's reviewer should have asked the question "hey do you think it would be more consistent to use friend instead of static public?" even before making any actionable requests. And if the answer is no and backed up with reasons, which it was, and if there's no rule in the coding standard on this point, then the reviewer should have said okay and approved it.

KKKKkkkk1 9y ago

It's nice to have the luxury to have a PR languish because of obdurate code reviewers.

throwanem 9y ago

I mean you could just explain why you're not going to do that, and if he still refuses to sign off because of it, find another reviewer or escalate or both. I agree that it's no good having a reviewer with bad priorities hang you up over them. But you're the one whose task it is to carry through if you can, and parsing blame doesn't ship code.

matwood 9y ago

If I'm reading this right, it sounds like the reviewer was arguing for consistency with the rest of the code base. If so, I can agree with the reviewer but I would have added that if the static function was superior how much work would it be to make them all consistent.

jschwartzi 9y ago
It would've required completely rewriting the solution. The reason we used friend functions in the past was when we had god objects that we needed to test. It did not make any sense here since the class under test had a single responsibility. It would also have made the implementation more complicated and would have tightly coupled the unit tests to the implementation of the class.
dahart 9y ago
Being a programmer, it's extremely likely that @jschwartzi was already trying to be consistent in every way he knew how, and every way that mattered to him. Bringing up a random and surprise way to make something consistent in a code review, for reasons not already written down and agreed upon in your team's coding standard, should generally be deemed inappropriate. I recommend codifying the axes of consistency in your org's coding standard and style guide -- that's what they are, after all. Make sure everyone agrees. If a new consistency rule seems important after multiple similar PRs, it should go through a process where the whole team discusses whether it should be adopted into the style guide. In my experience, "consistency" is one of those abused opinion-principles. Everyone has a different idea of what it means to be consistent. Some consistency is a great idea, but in general, appeals to consistency that aren't already in the coding standard should be prioritized lower than simplicity, and lower than whether the code & tests work as designed.
- matwood 9y ago
  The problem is that what I'm talking about is not style based. In a simple, made up case if I have a class that contains utility functions and I task someone to add a new utility function I would expect it to end up in the already existing class. Making a new utility class for the new function because the old class is too large in the new programmers opinion is breaking consistency. The class may be too large, but putting the new lone function off by itself is now an exception that everyone else will have to deal with. That one inconsistency then leads to others adding their own utility classes and then you have a big mess with some functions scattered across new utilityX, utilityY, and the old utility. As I said above, I would have handled the review differently and suggested/asked if all of the functions involved could be refactored to use the new improved method. In my example above, I would have asked if he/she could go ahead and break up the utility class as part of the change to add the new function. If the work was too much to bring it all up to date, then fit with what already exists and put a ticket in to do the explicit refactor later.
  - dahart 9y ago
    I understand and appreciate your point. And there are definitely situations where I agree with your take. Also I'm glad you'd phrase the review as a request and not a demand, that is closer to the right approach. But, as an engineering manager, I'm making a procedural point about code reviews, not disagreeing with your opinion. If the type of consistency being requested in the code review is not in the coding standard, then it's not fair game for blocking a PR or even making judgements, pure and simple. It's certainly okay to discuss it, and for the reviewer to ask questions and see if the submitter would like to improve consistency according to the reviewer, but if it's not part of the standard, then it's not a rule. "Consistency" is not a rule by itself anyway, it's a vague unspecified idea. There's no such thing as 100% consistency in multiple pieces of code, unless they're 100% copies. Every single file has unique differences that the team has to deal with. There's not much evidence that being extremely nit-picky about consistency improves safety or productivity, and in general, there's no reason in advance to think the "exception" will cause anyone else any trouble. Worrying about it may be premature optimization. To be more specific to the example above, there are times when visibility of functions should be limited, for safety reasons. Exposing functions selectively using friend declarations is generally seen as good practice in many situations, it's not a bad default choice. But, static public functions absolutely have their place too, and moreover they are a simpler language feature than friends. Simplicity is (to me) more important than consistency. In the example above, @jschwartzi's reviewer should have asked the question "hey do you think it would be more consistent to use friend instead of static public?" even before making any actionable requests. And if the answer is no and backed up with reasons, which it was, and if there's no rule in the coding standard on this point, then the reviewer should have said okay and approved it.