CINQby Quinten Stekelenburg
In my previous blog post - Refactor that ugly code (Part 1) - I talked about getting an ugly piece of code under test. Getting that code under test enables us to refactor it in a confident and safe manner. A safety net is in place. It is time to drive this thing home. In this post we are going to see if we actually can get rid of those pesky code smells.
At the end of part 1 we ended up with this code:
Now it is time to see if this code needs any refactoring. Let’s try to get a whiff of the code smells. Before we start, it is important to notice that the fact of code smells being present in your code base doesn’t necessarily mean that the code needs to be refactored. Code smells aren’t a bad thing, they are just signs that your code might give you headaches in the future. Afterall, why should we go on a refactor frenzy if the code never changes? If it never changes you might as well do it quick and dirty, commit, push and don’t look back.
Unfortunately (but fortunately for this blog, otherwise it would be a really short blog) in almost every case code changes. So we try to anticipate changes. We just don’t know exactly when they will come and what that change actually will be. At the risk of sounding a bit like captain obvious or come across as extremely boring, the easiest way to brace yourself for future changes is to keep your methods small. This doesn’t mean we dive into the code and start breaking it up and extract everything into its own class/method until we have a huge collection of one-liner methods. No, there has to be a reason or thought behind it. The number one reason to break things up in smaller pieces/modules is almost always: “each class should have its own responsibility or reason to exist”.
We don’t want hollow middleman classes that just pass everything on to the next class. Neither do we want big “God” classes that do everything by themselves. “But what do we want?” you might ask yourself then. When looking at a class, the most important question I ask myself is: “What is the purpose of this class?”. Another great question to ask yourself would be: ”Is this class doing too much?”. If so, we might need to do something about all those responsibilities.
To see if a class has too many responsibilities we need to determine what the purpose of this class is. Judging by the name of our class (AlarmService) it seems to be some kind of service class that coordinates the process of determining whether the alarm should be switched on or not. If I had to describe what a service class is/does, I would try to explain it with a real world analogy. A service kind of acts like a human traffic controller. The traffic controller blows his whistle, signals you to stop or shouts at you to drive your car in a certain direction when needed. What he certainly does not do is get in the car and drive it himself into the desired direction. Do you see the similarities? The service class shouldn’t be bothered with all that boring low-level stuff. A service should be delegating, that is what it is good at.
Looking at the service class in the code I see that the service has a state (the mutable field alarmStatus). This is a code smell and a red flag! You should never have state in a service class. There are several reasons for not having state in your service. For me, reusability and testability are the main reasons. Currently the alarm service is tightly coupled with the alarm state. The reusability is not as good as it could be. If we receive a new requirement which states that we want to be able to turn on the alarm manually for a reason not related to the pressure of a tire then we still have to use the entire AlarmService to achieve this. The method AlarmService.check() alters the state. AlarmService should not care how to turn on the alarm. It should delegate that to another class. Another thing that seems odd is the fact that the AlarmService itself is performing some sort of calculation to check if the pressure of a tire is within or outside a certain range.
To sum it all up we can see the following responsibilities for this class:
Ideally a class should have one responsibility. Although the AlarmService in this example is very small, it already looks like a Swiss army knife! It knows and does too many things. Not ideal if your code is going to change in the future. If we don’t do something about it now, the cost of changing your code in the future can turn out to be really high.
Another thing to look for in code is the readability of the code. Can we comprehend the code without reading the code over and over again? For example, if we look at the check method in the screenshot below we see that it isn’t a very long and complex method. But, besides the wrong naming of the threshold variables (variables that start with a capital letter?), that if-statement isn’t very reader-friendly so to speak. Every time I read it I am mentally reasoning what it actually is doing. This could be optimized. Let’s add this to the list of things to refactor.
Okay, enough complaining. Let's actually do something about that code. Based on the gathered responsibilities and weird things already spotted I came up with the following todo list:
First thing on the todo list: We want to break the hard dependency that the service has on the TireSensor class. We want that ugly “new TireSensor()” statement out of the code.
We could accomplish this by injecting the sensor into the AlarmService. My first thought would be to do that via the constructor. And this is where my first design decision comes into play. Because, a service is typically instantiated as a Singleton and if I then inject the sensor via the constructor I would never be able to use/check multiple sensors without instantiating multiple alarm services (which is very hard to do when the service in question is a Singleton). If I always want to check the same sensor during the lifetime of the app, injecting it via the constructor would be fine. However, in this case, I think it would be wiser to inject it via the AlarmService.check() method. We now have some flexibility and anticipate being able to measure multiple sensors in the future. Sounds like a good reason that justifies the decision to inject it through the method.
By the way, I say "justifies" which kind of implies that taking another decision is wrong. But that’s not the case at all. I rather prefer the phrase “each design decision has a trade off” instead of talking about right or wrong.
After altering and adding unit tests and changing my code, it now looks like this. We are providing the dependency to the service through the method. Sweet! One down, several to go!
Oh and by the way. You remember in my previous blog Refactor that ugly code (Part 1) we ended up with an inner TestableAlarmService class? We don't need that anymore! We can supply the mock through the check method. Cleaner tests! Win-win!
The next thing we want to improve is separating the actual alarm from the service. The AlarmService shouldn’t be responsible for keeping track of the state of an alarm. It’s time to create an alarm and release the service from that burden.
The way the alarm is working now is that you can turn it on once and you can never turn it off again. It sounds like that could use some improvement too. Let’s also change that while we are at it. STOP! This would be the point to stop typing, get away from the keyboard as far as you can and try to remember what the goal of this whole change was in the first place. The goal here is not to add any additional requirements and/or features. We are refactoring! Which means we want to improve the code (quality) without altering the behavior of the system. We can put this new idea on the backlog and carry on with our initial goal. After writing the tests upfront we can now start to implement the Alarm class.
And of course we want to use this alarm in our AlarmService. We inject it into our service. Via the constructor this time. This is also a design decision! I expect that we don’t need to use several different alarms when using the AlarmService. A constructor injection seems alright here. If this decision doesn’t turn out to be the right decision, it isn’t the end of the world! The main thing to keep in mind is that after refactoring you are always in a much better shape for maintaining your code than before we started this whole excercise. So, don’t be scared to make decisions. I would rather make a decision with certain trade offs which might be “wrong” but still improve your code quality. It is always better than doing nothing and having a crappy code quality.
We are progressing quite nicely. The alarm is extracted and injected, the sensor is injected… What else? Next up is moving the logic for the actual assessment of the pressure out of the class.
Now that we are on a roll we want to take away even more responsibilities from the service class. We could (and we should) put the logic/calculation, to determine if the measured pressure falls within or outside a certain range, into a separate class.
One of the other things which is also very important is naming things. The naming should be logical (well, duh!). As logical as it sounds, it is also a hard thing to do. You don't want to end up with long class names like “TheTireSensorPressureRangeChecker”. On the other hand, you don’t want to be too vague either, like calling it “SensorManager” or something along those lines. What helps me name things is to try and think of a real world example that resembles what you want the class to do or to represent.
“PressureGauge” seems to be a good name. And as always, we write the tests first! And we can do that fairly quickly because we already have the test cases in the AlarmServiceTest class from part 1. After that it is time to implement the gauge. In this implementation the gauge accepts a TireSensor, measures the pressure, assesses whether the pressure is too low, too high or optimal (based on the thresholds) and returns the result of the assessment (PressureState) to the calling client .
A practical tip: it is good practice to “return” to the calling class as soon as you know the answer, instead of storing your result in a temporary variable and “returning” at the end of the method. Another great tip is that if for some reason you do need to store the value in a temporary variable, place it as close as you can to the place where you need the variable (pressure variable is right above the if-statement where it is first used).
And just like we have seen with the alarm we also want to use this implementation and inject it in the AlarmService. And we're done! I already mentioned that naming things is important. I chose to rename the AlarmService into PressureMonitorService. This way the class name reflects the purpose of the service way better.
Note how we wrapped the delegation of the pressure assessment and alarm activation in a “pressureIsNotOptimal()” and an “activateAlarm()” method. The check method now almost reads like a normal sentence. The responsibilities are separated quite nicely and if another developer now looks at the code, he can immediately see what the intention of the class is and what the PressureMonitorService.check() method does.
By the way, I also removed the method getAlarmStatus(). It feels a bit out of place. Since we now have a separate alarm, other classes that are depending on the alarm shouldn’t have to call that method via the AlarmService. Every other class in the system that needs to know the status of the alarm should be depending on the alarm and not this service class. Important thing to keep in mind is that other classes in the system could already call the getAlarmStatus() method. If we remove the method we also have to fix that. So before you delete a method you should always check where that method is used. If you have to change the whole application when you delete the method then you might want to postpone this action and do this in another user story.
We refactored a lot but looking at the PressureMonitorService it was all worth it. We are ready for future changes! At the beginning we started with three classes/objects and no tests. Now that we are finished we ended up with twice as many classes/objects plus a fair amount of unit tests. The code has been split up in cohesive and logically bundled code blocks. The responsibilities are finally in the classes where they should be!
One last thing… When I show this code to other developers, there will always be a few that say: “But now I can’t see in one glance what the AlarmService is doing exactly”, “The calculation for assessing the pressure is in a separate class”, “The alarm and its logic is in another class”. "The code is scattered all over the place", “In the old code the only thing I had to do was open the AlarmService to know everything”.
My question to them would be: “Why do you want to know everything?”. You should not need to know everything in order to change something. If I had to choose between:
… I would always go for the latter! And if you do want to know everything in one glance, then you probably should have chosen a procedural language instead of Java to begin with.
As always, the code is available on my GitHub:
https://github.com/vlouisa/tire-pressure-kata/tree/tpms-service-refactoring
If you check out my code you might notice that the pressureGauge class looks entirely different than the one in this blog. That's because I was not done yet with my refactoring. I was experimenting and seeing if it is handy and/or useful to apply the Chain of Responsibility pattern to that class. But including that also in this blog is beyond the point I was trying to bring across. Zooming in on that though might be a nice sequel to this part. If you think this would be a great topic for another blog, let me know.