The black sheep code smell revolves around the concept of a class which contains a single method which is significantly different from the other methods and in essence sticks out like a black sheep.
In this code smell, we see a single method that is significantly different from the other methods in the same class. This is of course extremely subjective, but often times is quite obvious. This code smell leads us to find a method that doesn't actually belong in the class. It's presence probably violates the SRP (single responsibility principle) and causes the class to have too many responsibilities.
Sometimes we can detect this code smell by just the method signature. Let's take a first crack at it with an example. Look at these methods and see if you can determine which is the black sheep.
It may take a second, but the problem does stick out a bit. It's our findFreeItemId method. This can be a little subtle, but looking at the other methods, they all operate on items. Only the findFreeItemId doesn't actually operate on items. It's a utility method that finds a free itemId. The stock class which ultimately contains a list of items seems like the natural place to put such a method, but this class's job is to manage stock levels. Finding a free itemId is a responsibility for someone else. So this method should be extracted, and a helper class that deals with finding free itemIds should be implemented like so:
In our overly simplistic example, we end up with a class with a single method. You may argue that this seems unnecessary. But again, always consider the health of a system over time. Code never stays static. We are always writing new features and refactoring. Therefore making our code easy to extend pays off in the long run. For example, we could have this class implement performance improvements in the algorithm to find free id's such as memorization or other caching. We could also reuse this class for multiple collections and free each from containing nearly identical code. Always look at your entire code base holistically, and consider both the present AND the future.
The other way we detect this code smell is by looking at the implementation of the methods of a class. Here's that same class with a slightly different set of methods, and the implementations shown.
Here our black sheep method is quite obvious. It just looks SIGNIFICANTLY different. In this case it's much longer than the others, but it may also be different for other reasons, not just length.
As we look at the getItemReport method, it's like the others in that it operates on an item, but a quick glance lets us know that it's doing a much different job than the other methods, and then clues us in to how the presence of this method makes this class violate the SRP.
The other methods operate on how items are held in stock, and understanding information about that stock. But this method prepares a report about the item. This kind of informational preparation is much different than the job of the other methods. Yes, it's convenient to place this method here. All the information is handy. And remember this is an overly simplistic example, in more complex real world code, we'd have even more justification for putting this method here. More reasons for why it's convenient to just stick it here.
But the method doesn't belong here. This is a reporting method. It belongs in a reporting class. So let's extract that method and put it into another class:
In this case we had to add a new method to our stock class - getItemTransactions - to expose the data we needed. This again seems on the surface to be a bad move. We just created more code. Where before we had only 6 total methods in 1 class, we now have 7 total methods in 2 classes. More code is bad right?
Wrong. Small focused classes are always superior to bloated vague classes that group up methods according to convenience. So adding appropriate methods to the right class is good. Adding inappropriate methods to the wrong class is less effective. It may be convenient today, but it leads to bloat, it can lead to "death from 1000 cuts" and it puts a burden on us that we pay tomorrow when we are less equipped to pay it. This is otherwise known as technical debt.
As for our solution, we could have handled this in a few different ways. Here I added a method to get the transactions from an item. Perhaps we could have added a method that exposed the item itself, or even thought up some other solution. How to handle the refactoring isn't the important part here. What's important is that we focus on putting the right method in the right class, and keep an eye out for those black sheep methods.
Happy coding!
Signup for my newsletter at here.
Visit Us: thinkster.io | Facebook: @gothinkster
| Twitter: @gothinkster
Top comments (0)