0

I would like to know how manage a type of pull requests. One of my reviewers ask me for a lot and a lot of changes (3 or 4 days doing changes), all related with possible improvements in the code that I thought it is too much and it ends up in a "perfect" and "smart" code. It is difficult to discuss with a person who try to abstract a lot and type the perfect code... but I have a feeling that sometimes, simple is better and use a lot of concepts for small modules is "overkill" an idea and we should wait a little for the refactorization.. It means, I always thought it doesn't exist one way to do the things but I can be wrong and this is the "normal" way to the things and the refactorization always is better.

One example can be:

  • Create multi inheritance with multiple layers for a better abstraction. In this case, it can be a perfect the abstraction but for 3 classes that we don't know how they will scale it, we could wait a little to verify better if we need these abstractions.
Improve this question

3 Answers 3

Reset to default
8

Do you need to do everything that your reviewer asks of you? Usually, you don't. A good code review process has some back-and-forth. A reviewer might suggest a change, the developer might explain why that is or is not a good idea. If the reviewer's suggestions really improve the code, you should consider them. If you don't understand why the reviewer's suggestions might improve your code, ask them.

If you are concerned that they are asking for too much in one pull request, talk to them. Ask them if it's OK to create a separate issue for specific, non-essential refactoring improvements. Then you can complete the PR as it is, and move the improvements to a separate task.

Improve this answer
1

If such major issues arise at the code review stage, it is too late. It shows that your development process has problems. In a normal process first the main design decisions should be defined and discussed within a team. Then there is not much space for discussion.

Then I see 3 major cases.

  1. If you followed the previously agreed solution design, then nobody can ask you for changes that deviate from that defined design.

  2. If the reviewer is aware of design decisions, but finds then at the moment of review not more good, then this should be addressed separately, as a new change or new feature, but still there is no reason to require anything that is not defined in the design decisions.

  3. If you have misunderstood some design decisions, then you should accept such review and rework your code.

From your question I suppose that there was no solution design defined for your feature. Then don't change anything and discuss it in the team or with an architect responsible for your application. In some cases it can make sense not to merge your code and to rework it, in other cases the changes requested by reviewer can be just rejected, in some cases in can make sense to merge your code and to implement the requested change in the next iterations.

Improve this answer
5
  • I admit, in our issues we don t comment anything related "we will create this class and then this another one". We have a list of reqs and we implement these... The point here, my code "works" and it resolves the issue but ofc, it doesn t do in the same way that the reviewer thought, it can be better or not... well, in my opinion is subjective sometimes. and, is the PR the moment to do this? I finish rewriting all in the way that the other person wanted... It could be more effective and less "frustrating" to move the issue to this person and he develops it
    – mcduty111
    Mar 12, 2021 at 8:29
  • I don't mean that particular classes need to be defined in advance. I mean, that architecture should be defined in advance. There should be some common rules about base things. For instance, these rules can say if there are any modules in the application or, what layers exists, what layers are not desired and should be avoided, how do you deal with dependencies between classes, is it needed to define interfaces for injection or interfaces should be avaoided and classes should be used for injection, what kind of components need to be configured and where configuration should be avoided...
    – mentallurg
    Mar 12, 2021 at 22:33
  • ... what class hierarchies are desired and what should be avoided, in what classes one should implement business logic, how and when and where should one validate the data, what security aspects are important in your particular project and should be done when one implements a new service or end point, how should one approach security, authentication, permissions.
    – mentallurg
    Mar 12, 2021 at 22:37
  • There are no rules that are valid for every project. It is important to agree about what rules you want to have in particular project. Then when you start work on a new feature, you just do a high level description like: "I will implement a new service and keep in mind our rules AAA and BBB, I will implement 3 new entities and will follow our rule CCC. Data will be validated in the layer DDD according to the rule EEE. A new class will be subclass of FFF. To avoid undesired dependency from package GGG according to rule HHH a new class III will be implemented to provide functionality of JJJ".
    – mentallurg
    Mar 12, 2021 at 22:43
  • Then review of your pull request will be relatively easy. There will be no disagreements about major decisions in your code.
    – mentallurg
    Mar 12, 2021 at 22:45
0

The question is at this point: should this set of changes have been made? If you say the answer is “no” then there is trouble obviously. The answer “no” should have been given before the change was started. And the decision shouldn’t be part of the code review, but should be made outside.

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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