Isn't refactor on an existing and fairly new module a normal practice in software product development?

Question

Currently when working in a scrum team in a software service company, who has a clean business domain. I am working on adding new features to an existing module. As the existing module is legacy, and new features need to be integrated to the system all the times, I sometimes feel the need to refactor on the legacy code and also on the new code. For instance, sometimes I would like to just rename a Java class so that it is more close to the business domain.

However, my manager seems not like this at all and get fed up with even my micro-refactor such as renaming one class name or a method name. She tries to persuade me not to do that politely, but I indeed have difficulty to tolerate this as what she suggested are totally against what I have learned from Clean Code and the Domain Driven Development. On the other hand, I understand her worry in that the company's code review process recommends coders to do separate pull requests on actual code change and refactor, but to get separate pull requests merged will take extra time according to the company's code review process

Question:

1. Should I really follow my manager's command?
2. Is the company's pull request recommendation proper?

IMHO, if I follow her suggestion, the code base will gradually become the rolling big ball of mud

Answer 1

There is a danger, when picking up a new skill from a book, blog or video, to go "oh, a shiny new thing!" and use it everywhere.

Resist that temptation.

The problem with bringing new tools like DDD and clean code to an existing shop is that the shop already has a culture and history. You need to be sensitive to that. Name changes are especially problematic; the team you're on is already familiar with the names, and they've gotten used to finding things quickly by remembering those names.

Every change I make to a code base is carefully thought out. Is what I'm about to change going to be a net gain, or a net loss? For some things, the cost might outweigh the gain.

There are many ways to write good code. "Clean code" and "TDD" are just two of them. If you're finding yourself trying to bend the will of an existing code base to the philosophies of Uncle Bob and Eric Evans, first ask yourself: "Is the code already adequate, from an organizational standpoint" and "are the benefits worth the costs?"

Further Reading
Are bad programming practices typical within the software industry?

Answer 2

One of the most fascinating things I learned when I read Martin Fowler's Refactoring book was how many types of refactors come in pairs, one that is the exact reverse of the other. There aren't any refactors that are universally beneficial. It always depends on the circumstances, and often other developers are aware of circumstances you aren't.

Sometimes the merits of a refactor are not immediately obvious, and you need to make your case. That case shouldn't be something like "DDD says" unless the reviewers are very familiar with DDD. It should be something like, "The old name makes it difficult to tell this concept apart from this other one. This new name is closer to how our customers would refer to this concept in their requirements. This will make it easier for me to make x change I'm planning in a subsequent pull request."

In my experience, most developers with concerns like yours are not making bad changes, they just aren't good at selling them.

Putting it in a separate pull request also has a good reason. It's not just for the sake of bureaucracy. It's easier to review 10 pull requests than one big pull request that should have been 10. If I see a pull request that renames a class, and the author has made a good case for it, I can quickly glance through the changes and see "yep, this is just that rename," and hit approve.

Alternately, if you've stated your reasons well and I'm aware of circumstances you aren't, it's easier for me to correct you. "While your term is more popular broadly, in our particular industry the existing term is more common, for x historical reason." And I can reject the pull request without rejecting other changes I don't disagree with, or feeling like I'm going to make you feel bad.

If that rename is mixed with other changes, I have to painstakingly, line by line, determine if this line changed due to the rename or to something else, or both. If I have objections about the refactor, I may not want to look petty picking on a rename when there are meatier issues to consider, so I have to balance my review. That extra mental effort makes me annoyed about the rename, even if I would have supported it in a separate pull request. It's irrational, but that's how human brains work, and that's one reason why we make the rule about refactors in separate pull requests. It also feels more important if someone took the time to single it out.

In other words, there are not just technical factors to consider, there are human factors to consider, and you will be more successful if you find ways to help other developers be happy about your changes.

Answer 3

Symmathesy Its worth a read, and even finding a presentation on it by her.

In short the code base is not 100% checked into source control. It also exists in the heads of your fellow developers/business analysts/testors/architects. It is a lot harder to update these parts of the code base.

It is great that you have updated your own internal programming library. Before you apply this to the code though, you will need to update the libraries of your collaborators.

Imagine this scenario:

Frank is a new team and comes from a Functional background (if your functional choose a foreign sounding background). They take a look at your code base and immediately start to apply the rules of DDD and clean code. When you look at the code they have applied monads, functional decomposition, currying and a whole host of strange sounding patterns.

You as an experienced dev have been asked to implement shiny new feature on top of this code, and urgently too. Can you? Probably not, not because you lack experience, but because you have now been asked to read something indistinguishable from ancient greek.

This is essentially what you are forcing onto your colleagues. Your boss is right to ask you not to do this, they have to maintain a team capable of updating this code. They also need to be able to bring on new hands quickly enough to be useful when people move on.

Now if you can improve the code quality, then yes you should improve it. First though you need to be clear about what would improve code quality. Second you need to ensure that the team can still work with the improved code.

At my work we share books on these principles and discuss them. We then pilot a small project with the principles and compare them to what we were doing a year ago. Sometimes we move forward, sometimes we go back. We actively discuss what and how to remodel sections of the code base, and follow up with sessions about what was changed.

Answer 4

What the books you mentioned don't tell you is how to do get the team on-board with such refactorings.

If you are going to suggest a class or method renaming in a team, the only way of doing it properly is to ask at least one of the other senior devs what they think about it, and get a second opinion.

If they are fine with it, tell your manager that you all agree upon this change and make sure the renaming is properly communicated to all people who might have already worked with that part of the code base.