Putting extra code review burden on team because of refactoring

Question

I am reading Refactoring by Martin Fowler, and working in the patterns suggested into my coding. This has caused me to put up two code reviews for my service which are significantly lengthy, which are just refactoring code reviews. It is challenging for my reviewers to know what I am doing even though I have littered the CRs with descriptive comments. Is there a process to let my team not spend as much time on refactoring CR's? Or just general advice on this front?

Answer 1

Refactoring for the sake of refactoring, even if it's to better code, just simply isn't worth it, especially in a business context.

Like you've pointed out, your code reviews are lengthy and challenging for your team to adequately review. However, not giving them an adequate review isn't a viable option. What if you are unavailable (or leave) and these other people need to maintain the code? They may understand the current state of the code and can fix it. If they don't have the opportunity to review and ask questions, you would be the only one with an understanding of the changes and how they impact the system. Another pair of eyes or two is also helpful to make sure that you didn't introduce a bug (perhaps your test coverage isn't as as good as you think it is) or performance issues. You don't want to rubber stamp any pull requests.

My recommendation is to better work with your team for planning these changes. Since you've identified some refactoring that, in your opinion, will make the software easier to understand and maintain, you should log these somewhere so that way they are not forgotten and can be properly planned with the other work that needs to be done by the team. On teams that I've worked on and with, they've typically used the defect tracker for this, often with a ticket type or label or tag specifically for technical debt. You can get as detailed as you want in what you think should happen. In this case, perhaps link up the pull requests to the work, but don't expect them to get merged right away.

Instead of working on these code changes, in the future, I'd recommend simply logging the potential to do work and talking to your team to ensure that you are working on value added work. Perhaps the team will agree that this type of work is valuable. If it is, they will make the time to do the appropriate code reviews. However, if it's not valuable, it's waste (in the lean sense of the word - partially done work that isn't merged in for potential release and waiting are both wastes and lean seeks to eliminate wastes from development processes).

Answer 2

Refactoring is a technique, not a deliverable.

The code being reviewed should be easier to maintain, easier to extend, or easier to understand. If it is, then the refactoring is a "pay it forward" approach to being ready to deliver the next challenge, in less time. Such flexibility can sometimes assist your team to capture market share, or at least tolerate the next emergency.

Now if the code being reviewed cannot demonstrate such benefits (and I prefer demonstrations over verbal patter), then the refactoring may have made the code harder to maintain, harder to extend, or harder to understand. Refactoring alone is not an indicator of code quality, but how you move between different states of code quality.

If the benefits are obvious, then the argument against refactoring is just an argument against doing work. Such arguments probably led to the code being in a poor state. Now that it's time to pay off the technical debt, the team is asking if they can defer the payment another few months (or years).

If the benefits are unclear, then the argument against refactoring is an argument against doing work with dubious value. Such arguments are healthy and indicate the team is not keen on wasting time and effort.

The problem is one of assessment, but it is harder to assess the value when the complaint is about the technique and not the value. I would try to get the team's complaints to include some commentary on the value, and that should help you determine if the value is in question, or just the amount of work to be done.

To illustrate (hopefully with humor), I could complain about not wanting to tolerate any more of your walking and demand that we stop walking because it takes so much time and effort. That is both a silly argument or a good one, depending on whether you're walking in the right direction (assuming you even know what the destination looks like).

Answer 3

Refactors can be great, but they can also be tricky and should be tackled as such.

At work we follow the boy-scout rule which means whenever we touch code, we also try to clean it up within the scope of our work.

For example, if I'm working on fixing a bug in a single class, I might also look for typos, confusing method names, or redundant code. If I have a problem with the overall design and structure of that class and it's parents/children/clients, I will make a note of my issue with it to my team and on my pull request, but I won't go about changing it.

When it comes to refactors like updating how an extremely common class is used, or a common interface, or confusing logic, SRP, etc. I usually slowly build a list of those over time and once per sprint or 2 if anyone has extra cycles, they can take a couple points for a refactor where they tackle everything at once, and the whole team is aware and prepared for this.

Something to note here is there's no end to a perfect refactor... it's a rabbit hole that will always keep going. Therefore refactors should be well planned, clearly scoped, and time boxed accordingly.

Best of luck!

Answer 4

Top reasons to refactor:

1. Refactoring to make it easy to add a feature that has customer value.
2. Refactoring to remove a bad style choice that caused a high priority bug.
3. Refactoring to help put code under test that has interesting and hard to predict logic.
4. Refactoring to make this code readable.... wait, why are we looking at this code if we have no work to do here?

The 4th one is the one I always struggle with. I see a refactoring and I want to do it but can't justify it. Sometimes I refactor stuff like this and never turn it in because all I'm really doing is trying to clear my head. I dump it in a junk folder where it likely never sees the light of day again. If anyone asks me what I'm doing I say, "Oh just trying to grok the code base. Sometimes I read with my fingers".

Nothing wrong with this, when you have nothing better to do.