2

I'm not talking about the usual refactoring of new code you do as part of a development. This is more Boy Scout Principle type changes.

While I understand why they're all made in the same branch, it seems there is some mileage in keeping them separate:

  • The intention of the functional change is clearer in the maintenance phase
  • Code review for the functional change is shorter
  • It makes it easier to grok a module in a branch as there are fewer changes
  • The functional change can be submitted faster while the opportunistic refactoring can take place at a more suitable time

Any thoughts?

  • 3
    You could also argue that if you need a branch to do some Boy Scout refactoring then it’s already too big of a refactoring. Interesting question though. – Steve Chamaillard Jun 4 at 13:00
  • @SteveChamaillard Exactly! That's how I know I've gone down the rabbit hole and decide not to do a refactoring opportunistically (which I'm quite addicted to). – msanford Jun 4 at 19:14
1

I don't think that there's a hard rule here, you need to ultimately use your own judgement. The context of the organization will play a huge role in the decision.

Consider that my background is in highly regulated industries. Making changes beyond the scope of the defined issue has huge impacts to traceability that is required for compliance reasons. So when performing refactoring, we need to carefully look at the impact of the refactoring. If there's nearly 100% overlap between testing of the functional changes and the refactoring, then it may make sense to refactor at the same time as doing the functional changes. However, in cases where the refactoring would require more extensive testing to assure correctness, then the refactoring is typically recorded as technical debt and done appropriately.

Personally, I'm a huge fan of the Boy Scout Principle - always leaving the code in a better place than you've found it. But you do need to consider the downstream impacts:

  • How extensively is this code used? If it's relied upon across an application, then perhaps it may be a much bigger review and test cycle in order to ensure that the changes did not introduce a defect or regression.
  • What is the automated test coverage of the code being refactored, along with dependent code? Having automated tests and running those tests can definitely improve confidence in the refactoring and may enable more extensive refactoring as part of functional changes.
  • Will this make a code review of the functional changes harder? I want to make sure that the code reviewers can focus in and understand the changes that I made. In some cases, maybe refactoring will actually make the code easier to understand and follow.
  • Do we need to supplement the automated testing with any manual testing? Do the manual testers have the knowledge, ability, and time to properly review and test the extent of my changes sufficient to ensure quality?
1

If you stumble on an area that has technical debt, the first thing to do is add something to your backlog characterizing the problem. This is not an area you are actively developing on. It's also work that needs to be tracked and accounted for.

A well functioning team will set aside a certain amount of capacity to pay down technical debt along with the features and bugs. Your sprint planning will assign the set of features, bugs, and technical debt that get addressed in the upcoming sprint.

A big challenge with some technical debt like this is that you may not know just how much impact your changes are going to make. In general:

  • Document your assumptions
  • If your assumptions are wrong, and the impact is big,
    • Drop your branch, document the failed assumptions and put the issue back in the backlog
    • Create any new technical debt items needed to enable fixing this one, documenting the dependencies
  • Otherwise, fix it and merge it in to the develop branch
  • "A well functioning team will set aside a certain amount of capacity to pay down technical debt along with the features and bugs" I'd love to be in your world one day. In mine, product owners and marketing teams do not care about technical debt, and they especially don't want to change something that's already working. And since we don't want to work aside our work hours, we end up refactoring when it makes it easier to develop a new feature. – Steve Chamaillard Jun 4 at 13:17
  • @SteveChamaillard that's my experience as well. Most of my refactoring happens before adding a feature. It's easier to predict the future when it's already here. – candied_orange Jun 4 at 13:19
  • 1
    Sometimes you just have to be strategic. XYZ has to be fixed before we can implement this feature you really really want. But I've negotiated with my product owner a certain number of points per sprint/release to use for paying down debt. It also helps when they can see real benefit (i.e. cost savings, mitigating legal repercussions, pending updates to the desktop image will break the app, etc.) All of which I have to deal with regularly. – Berin Loritsch Jun 4 at 18:59
  • I wish the number of points were higher, but at least I have something. – Berin Loritsch Jun 4 at 18:59
1

Every impediment to opportunistic refactoring that is introduced is going to reduce the amount of refactoring actually done. This sort of refactoring is most likely going to be in the same files and likely the same functions of any functional changes, trying to segregate it to multiple changesets isn't going to happen often in practice. If you notice that foo() is a mess while working on bar(), that really isn't an opportunistic refactoring event and should be a separate ticket with a second changeset(s). If rewriting bar() while adding a new requirement makes sense then it should absolutely be done with that change, thinking you will go back later and fix it almost never happens. What should be done is drawing a line of what amount of refactoring time is acceptable as part of making a change, this needs to be a reasonable amount of time if you actually want to make a dent in technical debt.

  • Actually the reason you noticed foo() is a mess while working on bar() might be because one depends on the other. That's a good argument for fixing both. The danger is letting a chain of these good arguments turn into a full blown rewrite. What should limit your scope is the patience of your code reviewers. – candied_orange Jun 4 at 13:36
1

I see this question as addressing an informal vs formal issue.

I've never believed in formalism for it's own sake so long as you're responsible.

To be opportunistic you have to be exploiting an opportunity to pay down technical debt for a cheaper expense than if you'd filled a full-blow, self-standing, ticket against this issue. That ticket would require the full prioritizing ceremony. That doesn't make it cheap.

I'll argue that improvements can be made to code that is having a new feature added that isn't strictly required for that feature. However, don't be a cowboy. Talk to at least one person about this before hand. Because what you think is an improvement might not be to others. Be sure you haven't stepped outside of the scope of tests that will be run against this change. Keep in mind that large changes, ticketed or not, make it hard to figure out where a problem was introduced.

Do that and I won't force you to do it all in a separate branch. If you think you've just cleaned up camp, fine. Present it for inspection and be prepared to put back the rocks you thought you could take home. Think about doing that before you decide a separate branch is a bad idea or how many rocks you want to turn over.

P.S. I have a lot more tolerance for this kind of thing when it doesn't all happen with one commit.

  • I'd suggest that informally making code changes on the fly is the very definition of being a cowboy - regardless of whether you make a real improvement or not. But I say that coming from an industry that favors consistency over change, and that our code is shipped to customer site and the field engineers that support the product have many different backgrounds and levels of software experience - so consistency is a feature. – Peter M Jun 4 at 14:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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