6

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?

Improve this question
2
  • 4
    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. Commented Jun 4, 2019 at 13:00
  • 1
    @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). Commented Jun 4, 2019 at 19:14

5 Answers 5

Reset to default
2

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?
Improve this answer
3

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.

Improve this answer
1
  • 3
    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. Commented Jun 4, 2019 at 13:36
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
Improve this answer
4
  • "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. Commented Jun 4, 2019 at 13:17
  • 1
    @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. Commented Jun 4, 2019 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. Commented Jun 4, 2019 at 18:59
  • I wish the number of points were higher, but at least I have something. Commented Jun 4, 2019 at 18:59
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.

Improve this answer
1
  • 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. Commented Jun 4, 2019 at 14:00
0

The original poster asks a very important question

A few questions to ask the programmer

Is the refactor contained entirely inside the code that relates to your bugfix or new feature? How important is risk? How robust is the testing architecture after you make your change? Has your software been released? How dependent are your fellow developers in the code you are refactoring? Have you released the software yet? Why do you use branches? Do you do code reviews per branch change (will someone have to look through a huge refactor & the feature change)? How important is the refactor? How important is it to isolate the changes in history? How much do you dislike very large code reviews? How important is clean code?

Many opposing forces!

Switching branches in git has a small cost that may impact on developers desire to refactor in separate branches. If this is your barrier to creating a second branch a good solution is to use multiple working directories in git, some for refactoring and one for your feature change. The idea is then to make a new branch for the refactor in one branch and merge it with your feature branch. This way it separates your refactoring changes with your feature change code making merging easier by more smaller changes rather than one very large one. It is still a cost to create that new branch so may still be an impedement to change.

The refactor branches should push changes and the feature branch (the one that requires the refactor) should pull the changes.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.