How much refactoring is acceptable?

Question

I am currently in a project where one of my developer colleagues constantly refactors stuff on every ticket he's doing. We are using agile methodologies. I know that refactoring is a good thing to do while solving some issues, keeps the codebase nice and clean, but my question is, what's the limit, how many files should one be touching at max? Can too much refactoring bite us on the long term?

Sometimes it feels like he could be introducing bugs (although the system is heavily tested) and that it makes the review process quite cumbersome; reviewing changes in 60-70 files when the original change should have affected only about 10-20.

On the project the tickets we are getting are quite small, usually no more than 10-20 files need to be touched, and this developer is averaging 50 or so. It feels wrong, and I haven't found a way to convince him that it's not. He says everything is tested, so all changes only improve the codebase. Finally, I am assuming that this refactoring could also be slowing him down, cause he just changes so many damn stuff all the time ! :D

Any thoughts ?

My personal view is that refactoring should be done, but it should usually be limited to the classes that are changing for the ticket anyway, and should a need for a bigger refactoring occur, it should be a ticket itself. In some cases, I find myself not even changing small stuff, cause "I've refactored enough for this ticket as it is.", it feels like when I reach the refactoring limit I've got set in my mind, I just stop making any non-necessary changes to the codebase and only focus on wrapping up the ticket..

Edit: I've come to realize from reading the responses and similar questions on programmers stackexchange (that I just discovered, what a lovely place !) that my main problem is that what I described is a single commit. I need to tell everyone that that are free to introduce refactoring but make sure they keep it in separate commits to ease the reviewing process.

Edit: Another aspect that I have found to be problematic is merge conflicts. These refactorings end up costing time to other members working on same pieces of code every now and then. Nothing substantial, but if everyone on the team made so much refactoring we would surely have many more merge conflicts. Not saying that this is the end of the world, just putting it on the table.

Answer 1

I wouldn't focus on the quantity, but rather on substance. The number of affected files during a group of refactoring operations is as irrelevant as the LOC you write per day. An extreme example is the renaming of methods to follow a convention. It may affect thousands of files, but is barely more important than a refactoring focused on two files which radically change their design.

By focusing on what are the group of refactoring operations improving rather than how much files or LOC are affected, you also reduce the complexity of the problem you are currently encountering. During the code review, look at the overall impact of those operations. Do they improve the code base? How much?

Consider refactoring operations individually. This will help your team determining precisely what is considered useful, and what should be avoided. For instance, imagine that during a code review, you notice that your colleague made the following refactoring operations:

1. Split methods which were doing too much,

2. Introduced abstract factory pattern,

3. Replaced a few ifs by inheritance, simplifying the logic,

4. Replaced a few inheritance occurrences by ifs, reducing the number of classes and LOC and making the code easier to read,

5. Rewrote the method findSparse to be more readable,

6. Renamed a few methods to show that those are actually getters and setters,

7. Replaced a singleton and introduced Dependency Injection: the code is now much easier to test.

Rather than telling that six changes affected 28 files and should be ignored, since they are above the threshold of 25 files, your team may rather consider that:

• Refactoring operations 1 and 7 are totally worth it.

• Operations 2, 3 and 4 are interesting, but cost much time. They shouldn't be done if the project is under time pressure, but should be otherwise.

• Operation 6 is cosmetic and has a risk of introducing bugs on the part of the project which uses Reflection (explaining the risks which outweigh the benefits is essential). This operation should be postponed until all the team is ready to undertake the change.

• Operation 5 is not improving anything: the author of the refactoring just rewrote the code to match his own style. Most team members agree that this doesn't make code more readable, and some claim that they actually find the original version easier to understand. Similar operations should be avoided in the future.

By focusing on individual operations, you make it very easy for your coworker to understand the team's point of view, what is welcome, and what is not. By focusing on quantity, you make it more “I don't like your way of working” nonconstructive criticism.

Note that:

• If commits are granular enough (and they should be; for the refactoring listed above, I will expect at least seven commits, and the eighth one implementing the feature itself; ideally, point 3 and point 4 should also be subject to multiple commits, leading to more than ten commits overall), rolling back a commit shouldn't cause too much trouble.

• If commits are sparse and monolithic (that is, one or two commits for the refactoring operations listed above), there might be a more serious problem in your team. Try to explain them that one commit should correspond to one change, as small as it could be while remaining independent of other changes.

Answer 2

Agree up front how much refactoring is to occur on each ticket. This is done as a team. If that developer wants to do extra stuff, bring it up and decide whether it is OK. Keep things scoped properly. There's nothing worse trying to explain to a boss why the team broke something that wasn't in scope.

Also, verify refactoring is actually improving the code base. Your team should have done some code metrics on your system identifying the hotspots. Some developers just like to re-write everything, make sure the efforts are actually improving code base. Sometimes its just not worth the effort or the work being done isn't actually improving anything. Again, measuring before and after will help.

Praise the members of the team doing the extra work. It's good that they take pride in improvements. Try to show them that we want to capture all the good work they are doing and not create any surprises. They may resist on defining scope, but one definately doesn't want a bunch of loose cannons in the code base because at some point it may come back to bite you.

Answer 3

how many files should one be touching at max?

Theoretically speaking you can go on refactoring indefinitely because there is no perfect codebase.

One way to get around this that has worked out for me and my team is to identify these areas that need cleaning, make tickets out of them (call it technical debt tickets or anything else along those lines) then during your sprint planning attach these tech debt tickets to your main ticket.

This way you are scoping your work and not doing any additional refactoring that might cause trouble to other developers (since you are not stepping on their toes).

In the above approach it is obvious that you will not be capturing every single, little area of code for refactoring as tech debt, so for those little changes you can all include them under the ticket that you are working on. That should be fine.

However, small changes shouldn't add 20+ files over what you have expected, and if you do see that happening then that means 2 things:

1) The person is doing WAY too many random things unrelated to the ticket.

2) The person is doing a big refactor which was not captured previously.

Either way you can definitely bring these two scenarios up for discussion.

For the first case you can say the following:

Making quite a few amount of changes unrelated to the ticket is more distracting than helpful in terms of leaving your codebase cleaner than you have found it.

This mantra is about the code you are working with currently so if you are working with class A and it is related to class B which is a bit messy, then refactoring it would make sense.

But if you are working with class A and you are refactoring class Z which is totally unrelated then not only are you creating more need for testing, but you are also distracting and slowing down the overall development process (slowing down dev testing, unnecessary context switching, more testing resources, introducing more entropy to the workflow).

For the second case you can say the following:

Working on a large amount of tech debt which was not captured previously is a betrayal of the process and of the team since it was not agreed upon by all and wasn't prioritized by the business analysts as something to spend time/resources on.

For the next time when one finds a large chunk of technical debt one should capture it and add it to the backlog for later prioritization instead of tackling it with no planning and direction in mind.

Answer 4

To me this sounds like you and your team mate aren't on the same page when it comes to the level of quality required of the project.

I recently had the same problem with a team. What worked for me was determining the ideal design we all agreed on for every part of the project. The repository would use CQS, the MVC controllers would be very thin and primarily use the services layer etc.

Once every team member agrees on the design of the project, formalize it by, for example, some UML diagrams. Print them so you have them available in the team room during code reviews. When the design is clear, it's easier agree on when good is good enough and more refactoring isn't required.

Also, I would recommend only refactoring parts of the code relevant to the feature/story you are currently working on. That way, the refactoring time is small compared to the entire feature and you can 'sell' the refactoring time more easily to project management.