“How did you refactor that?”
After my video about “Primitive Obsession”, a viwer asked me on Twitter how exaclty I refactored the code towards the final result.
Which steps did I take? Which transformations did I perform in the code?
In this video, I want to show you how I would refactor this piece of code to get rid of the primitive obsession.
Intro (TL;DR)
My name is David and I am a trainer and technical agile coach with over 12 years of experience.
Today I want to show you how to refactor some code that over-uses primitive data types. I will not talk about why this is a problem - I already did this in the video “Primitive Obsession”, and I have added a link to that video in the description.
Now, I only want to get rid of the problem.
So, let’s look at the original code again.
By the way, if you like this video, please subscribe and share it with your friends and followers. That would be awesome.
Question / Problem
It is a controller from an application that helps people pay their taxes. The controller gets a tax account number, validates it, reads the tax office ID from it and then displays information about the tax office.
If the first two digits of the tax account number - which are the tax office ID - are “46”, it will display “Linz” as the tax office name and the IBAN of the tax office in Linz.
I have added some more code compared to my last video. I have added a view for this controller. It is mostly empty, but here you can see how the controller would be called from the view.
The controller registeres a TaxAccountNoChangedListener with the view, and every time the text field changes, the view will call all these listeners. Then, the method onTaxAccountNoChanged of the controller will do it’s thing, and this is the method I want to refactor.
In addition to what I showed in the last video, I have added a constructor with dependencies. And I have added some more validation: The method now outputs an error when the tax office was not found.
public void onTaxAccountNoChanged(String taxAccountNo) {
String errorMessage = TaxAccountNoUtil.validateTaxAccountNo(taxAccountNo);
if(errorMessage != null) {
payTaxesView.showTaxAccountNoValidationError(errorMessage);
} else {
String taxOfficeIdInput = taxAccountNo.substring(0, 2);
Integer taxOfficeId = Integer.valueOf(taxOfficeIdInput);
TaxOffice taxOffice = taxOfficeRepository.findByNumber(taxOfficeId);
if(taxOffice == null) {
payTaxesView.showTaxOfficeError("The tax office \""+taxOfficeIdInput+"\" does not exist.");
} else {
String taxOfficeName = taxOffice.getName();
String taxOfficeIban = taxOffice.getIban();
payTaxesView.showTaxOfficeName(taxOfficeName);
payTaxesView.showTaxOfficeIban(taxOfficeIban);
}
}
}
I needed this to be able to add some tests. And I needed some automated tests to show me that I did not break anything.
Remember: Refactoring means changing the structure of the code without changing the functionality. It means applying small, safe transformations to the code. And then, after every small step, I must check whether I did not change the functionality.
Yes, I can never proof or be 100% sure that I did not change any functionality. But when I work in small, safe steps and use the compiler and some automated tests to verify every step, I can be pretty sure. I would even argue that when I did not do that - work in small, safe steps and verify every step - then it would not really be refactoring, just some changes to the code.
So, I needed those tests before refactoring but how I wrote them is not important right now. That’s maybe a topic for a later video.
Solution
So, small, safe steps. And I want to get rid of the primitive data types.
As a first step, I introduce an object for taxAccountNo. I give it an ugly name for now, to remind me that I am not done here: I want to later remove the string from most parts of the code.
I press Alt+Enter to and let my IDE create the field automatically. I also create a getter (by pressing Alt+Enter again), but I rename it to “asString” because this is the string representation of that object. I do not use the name “toString” here because toString should be human readable and is not supposed to be used by other code.
Now I replace all other uses of the method argument taxAccountNo with my newly created object. The tests still pass. Now I rename the parameter and the variable so that their names indicate better what they represent. Now, the parameter gets the ugly name because it contains the “uglier” value.
I press Ctrl+Alt+Shift+T here to open the “Refactor This” menu. I am sure other IDEs have similar functionality.
public void onTaxAccountNoChanged(String taxAccountNoUnvalidatedUserInput) {
TaxAccountNumber taxAccountNo = new TaxAccountNumber(taxAccountNoUnvalidatedUserInput);
String errorMessage = TaxAccountNoUtil.validateTaxAccountNo(taxAccountNo.asString());
if(errorMessage != null) {
payTaxesView.showTaxAccountNoValidationError(errorMessage);
} else {
String taxOfficeIdInput = taxAccountNo.asString().substring(0, 2);
Integer taxOfficeId = Integer.valueOf(taxOfficeIdInput);
TaxOffice taxOffice = taxOfficeRepository.findByNumber(taxOfficeId);
if(taxOffice == null) {
payTaxesView.showTaxOfficeError("The tax office \""+taxOfficeIdInput+"\" does not exist.");
} else {
String taxOfficeName = taxOffice.getName();
String taxOfficeIban = taxOffice.getIban();
payTaxesView.showTaxOfficeName(taxOfficeName);
payTaxesView.showTaxOfficeIban(taxOfficeIban);
}
}
}
Now I want to move the validation into a factory method of TaxAccountNo since it should never be possible to create a valid object from an invalid input. I extract a static factory method from the constructor call and then I move this method to the TaxAccountNo class.
I will move the validation code into this method soon, but first I want to create a second variable for the sanitized input that will be available after validation.
public void onTaxAccountNoChanged(String taxAccountNoUnvalidatedUserInput) {
TaxAccountNumber taxAccountNo = TaxAccountNumber.fromString(taxAccountNoUnvalidatedUserInput);
String errorMessage = TaxAccountNoUtil.validateTaxAccountNo(taxAccountNo.asString());
if(errorMessage != null) {
payTaxesView.showTaxAccountNoValidationError(errorMessage);
} else {
String taxOfficeIdInput = taxAccountNo.asString().substring(0, 2);
Integer taxOfficeId = Integer.valueOf(taxOfficeIdInput);
TaxOffice taxOffice = taxOfficeRepository.findByNumber(taxOfficeId);
if(taxOffice == null) {
payTaxesView.showTaxOfficeError("The tax office \""+taxOfficeIdInput+"\" does not exist.");
} else {
String taxOfficeName = taxOffice.getName();
String taxOfficeIban = taxOffice.getIban();
payTaxesView.showTaxOfficeName(taxOfficeName);
payTaxesView.showTaxOfficeIban(taxOfficeIban);
}
}
}
I want to change the error handling to Exceptions, so I first duplicate the validation and add an exception.
public static TaxAccountNumber fromString(String taxAccountNoUnvalidatedUserInput) throws TaxAccountNumberFormatException {
String taxAccountNoSanitized = sanitize(taxAccountNoUnvalidatedUserInput);
String errorMessage = TaxAccountNoUtil.validateTaxAccountNo(taxAccountNoSanitized);
if(errorMessage != null) {
throw new TaxAccountNumberFormatException(errorMessage);
}
return new TaxAccountNumber(taxAccountNoSanitized);
}
I could later inline the validation method from the util class; and if this was the last method on that class, I could remove the util class completely. But that would require some changes to the validate method.
public void onTaxAccountNoChanged(String taxAccountNoUnvalidatedUserInput) {
try {
TaxAccountNumber taxAccountNo = TaxAccountNumber.fromString(taxAccountNoUnvalidatedUserInput);
String taxOfficeIdInput = taxAccountNo.asString().substring(0, 2);
Integer taxOfficeId = Integer.valueOf(taxOfficeIdInput);
TaxOffice taxOffice = taxOfficeRepository.findByNumber(taxOfficeId);
if(taxOffice == null) {
payTaxesView.showTaxOfficeError("The tax office \""+taxOfficeIdInput+"\" does not exist.");
} else {
String taxOfficeName = taxOffice.getName();
String taxOfficeIban = taxOffice.getIban();
payTaxesView.showTaxOfficeName(taxOfficeName);
payTaxesView.showTaxOfficeIban(taxOfficeIban);
}
} catch (TaxAccountNumberFormatException e) {
payTaxesView.showTaxAccountNoValidationError(e.getMessage());
}
}
I want to continue with the onTaxAccountNoChanged - method for now. The next type I want to get rid of is the taxOfficeId. I basically apply the same steps again: I create a new class, extract a factory method and move that method. But now I also extract another method, getTaxOfficeId and move it to TaxAccountNo.
public class TaxOfficeId {
private final Integer taxOfficeId;
private TaxOfficeId(Integer taxOfficeId) {
this.taxOfficeId = taxOfficeId;
}
public static TaxOfficeId fromString(String taxOfficeIdInput) {
Integer taxOfficeId = Integer.valueOf(taxOfficeIdInput);
return new TaxOfficeId(taxOfficeId);
}
public String asString() {
return String.format("00",taxOfficeId);
}
public Integer asNumber() {
return taxOfficeId;
}
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
TaxOfficeId that = (TaxOfficeId) o;
return Objects.equals(taxOfficeId, that.taxOfficeId);
}
@Override
public int hashCode() {
return Objects.hash(taxOfficeId);
}
}
I can now use the new class TaxOfficeId in the query method of the repository. But now I also have to change a test, since the old method is not called anymore on the mock object.
public void onTaxAccountNoChanged(String taxAccountNoUnvalidatedUserInput) {
try {
TaxAccountNumber taxAccountNo = TaxAccountNumber.fromString(taxAccountNoUnvalidatedUserInput);
TaxOfficeId taxOfficeId = taxAccountNo.getTaxOfficeId();
TaxOffice taxOffice = taxOfficeRepository.findBy(taxOfficeId);
if(taxOffice == null) {
payTaxesView.showTaxOfficeError("The tax office \""+taxOfficeId.asString()+"\" does not exist.");
} else {
String taxOfficeName = taxOffice.getName();
String taxOfficeIban = taxOffice.getIban();
payTaxesView.showTaxOfficeName(taxOfficeName);
payTaxesView.showTaxOfficeIban(taxOfficeIban);
}
} catch (TaxAccountNumberFormatException e) {
payTaxesView.showTaxAccountNoValidationError(e.getMessage());
}
}
The last step now is to create new classes for the tax office name and the IBAN. Then I can rename the methods for diplaying to only show. But I also have to change the tests again.
@Test
public void showsTaxOfficeInformationIfTaxOfficeWasFound() {
TaxOffice taxOffice = mock(TaxOffice.class);
when(taxOffice.getName()).thenReturn(new TaxOfficeName("Linz"));
when(taxOffice.getIban()).thenReturn(new TaxOfficeIban("AT03 0100 0000 0552 4464"));
when(taxOfficeRepository.findBy(TaxOfficeId.fromString("46"))).thenReturn(taxOffice);
controller.onTaxAccountNoChanged("46-999/9460");
verify(payTaxesView).showTaxOfficeName(new TaxOfficeName("Linz"));
verify(payTaxesView).showTaxOfficeIban(new TaxOfficeIban("AT03 0100 0000 0552 4464"));
}
I can now, as a final step for this video, move some value classes to the class TaxOffice and give them nicer names.
Conclusion
Now look at the code. This does look nicer than before.
I could refactor even further. For example, the classes for the value objects all have very similar functionality. They all wrap a single primitive value. They all have a hashCode and equals method and they look very similar.
I am sure I could unify or simplify some things here. But for now, I am done here.
CTA
What do you think of these refactoring steps? What would you have done differently? Please tell me in the comments or on Twitter, I am @dtanzer there.
And if you liked this video, please subscribe and share it with your friends and followers using the share buttons below - That would be awesome!
