My team has a challenge that I haven’t run into before. We all want the code to be better quality that it is now. However we don’t agree on what “better” is.
Even simple things can lead to long discussions and debates so it was a struggle to get some serious progress. Reviews could get bogged down because a refactoring someone did triggered a debate on its merit. We needed a format to allow us to work through some of the fundamentals and find where there was consensus. So I booked us in to do some mob programming together except we were not going to write new code. We were going to focus on only refactoring.
I booked in three hours for our first session. I had a system setup and some code already in mind. But we never got to changing the code at all. The time was spent discussing when it’s okay to do some refactoring and when it’s not. We decide on some simple questions to guide the decision:
- Are we going to change this code anyway sometime soon?
- Is this code that we read a lot?
- Is this code that interacts with a lot of other code?
- Is this code we are likely to want to reuse in the future? (perhaps for a different product)
If the answer was No to all of those questions then don’t refactor the code. It might be messy but as far as we know it works.
We also set down some guidelines for how big of a change we are comfortable committing to before asking the team for permission. How big of a change are we comfortable committing to before asking the business to clarify priorities. Useful things to know but your context would change the outcomes. But we wanted to continue these exercises so I booked us in for another 2 hours some time later.
Session two we got to looking at the code I had prepared. We talked about the options for our very first step and wrote up about 12 options to consider. In the end the team wanted to look through this function (all 159 lines of it) to see what it was doing. As we went through we discovered the different roles of variables and sections of code. But we also had lots of questions where at first one might have assumed returning True meant success we soon saw it had some more complex meaning as it could return True when the parser had failed. So I suggested we should comment our discoveries and questions as we go. That, if nothing else, our comments would be a valuable refactoring to help the next person along. We also tested our tests. I suggested we change one of these return statements from True to False to see what happened. Nothing. All of the tests passed. Our confidence in the existing tests dropped with that simple, random, test. By the end of our session – only two hours this time – we had gone through the whole function, raised a bunch of questions and answered some. We committed the changes and planned to continue with another session together.
Finally we got to the third session and we started writing code. At first it was just splitting out some variables into more localised ones with more useful names. But then we saw we needed some helper functions so we started a TDD process writing those new functions. But why are we writing new tests!? It’s just refactoring right? We discovered in our previous session that the test coverage could not be trusted 100% so I wanted any new code to be fully tested before it was introduced. That meant doing things we wouldn’t normally do like making those functions public. The first helper function was so that it would return an object instead of returning three different things using references. We had some good discussions on why that was a code smell but ran out of time before completing the new helper.
These sessions are continuing still. It’s been slow going but we’re making progress. I thought we needed to just agree on some basics on what “better” code looks like. But it turned out we needed to worth through some fundamentals on when refactoring is a good thing to do.