We return to our pizza shop to refactor.
I like to check out my "TODO" or "Smelly" comments and see what I can clean up. I normally go for "TODO" as most tools have a way to list them for easy access.
Visual Studio has a "Task List".
I have 5 TODO's. Five things don't quite smell right. Let's fix these before digging into more requirements. Refactor early and often. Clean up the code to produce µObjects that can be used in more places. Keeping code clean is just like housework; more and often makes it quick and easy.
In the ReplaceLastOfText we check a value against -1. What's that value mean? As engineers, we know it means not found; but... let's make it clear. Let's remove that mental translation from -1 to not found.
While looking at the ReplaceLastOfText class; I see it's doing a few string operations... Those should be encapsulated. I'll make a TODO and come back to them.
Fixing the first TODO; we now have private const int NotFound = -1; in commit 9207c83f.
The next TODO in the list is about some logic in a constructor. We never want logic in the constructor. It's a HUGE smell. We want to avoid anything that could ever prevent our class from instantiating correctly.
We have
public SentenceJoinToppings(IToppings toppings) : this(new SentenceJoinText(toppings.Select(t => t.Name()))) { }
public SentenceJoinToppings(IText origin) => _origin = origin;
The LINQ Select is some logic which we can't allow. We could cheat and make it a lambda or a delayed execution. There are similars in a place or two. At work I think we have a few more. It's a cheat. It's a workaround for a poor design. It's a class without being respectful enough to make it a class.
Let's see how we can make it a class.
We have some behavior; Turning it into an enumerable of IText. Let's make a class to do that for us. It's gonna do one thing; turn an enumerable of ITopping into an enumerable of IText. A great example of a µObject. We're not really doing this one for reuse; it's to have a class do one thing. If we can reuse it; great. There isn't likely to be any changes or branching logic in reuse. If we can use the class again, it doing one things allows it to be reused AS IS. If we do something else similar we can refactor into common code doing ONE THING. It's much easier to identify and combine commonality when each class is doing just one thing.
Here's our new SentenceJoinToppings constructors.
public SentenceJoinToppings(IToppings toppings) : this(new ToppingToStringEnumerable(toppings)) { }
public SentenceJoinToppings(IEnumerable<IText> toppingNames) : this(new SentenceJoinText(toppingNames)) { }
public SentenceJoinToppings(IText origin) => _origin = origin;
It took the introduction of a class to do that logic; and we're free from constructor logic.
public class ToppingRebaseToText : IEnumerable<IText>
{
private readonly IToppings _toppings;
public ToppingRebaseToText(IToppings toppings) => _toppings = toppings;
public IEnumerator<IText> GetEnumerator() => _toppings.Select(topping => topping.Name()).GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}
Simple and all ti does it transform an IEnumerable<ITopping> and transform it to an IEnumerable<IText>. This fix is in commit dfc53684.
The next one is about chaining in our PizzaDescription //TODO: Smelly - Chain?. This is shorthand suggesting the code be elvaluated into a chain of responsibility pattern.
We have a series of descisions and operations that are order dependent. For µObjects, Chain of Responsibility is how we've done this, formerly, procedural code into µObjects.
There's normally a bit of churn on the method and supporting classes as chaining thigns out forces certain object oriented adjustments which may not be noticed in other situations.
The PizzaDescription has changed a bit. Go check out the file in commit 5bcbf6e0 for the full details.
Our PizzaDescription now has a simple constructors that looks like
public PizzaDescription(IPizzaType type, IToppings toppings) :
this(new PizzaDescriptionAction(type), toppings)
{ }
public PizzaDescription(IPizzaDescriptionAction pizzaDescriptionAction, IToppings toppings)
{
_pizzaDescriptionAction = pizzaDescriptionAction;
_toppings = toppings;
}
Getting the description is a simple single line of
public void Into(IWriteString item) => _pizzaDescriptionAction.Act(item, _toppings);
This "Action" is what we currently suffix our chains with. They do an act.
Go check the above file out. There'll be some confusion; I'm sure. The next commit 36241fe is moving them all to separate files.
The next cleanup refactor is around our encapsulation violation. As a short term fix; I'm siwtching the CTORs to take a string; and Topping will store it as such. The ideal is to have Topping#Into(IWriteString item). I haven't gotten the rest of the consumers there yet.
Working on the recently identified smell in ReplaceLastOfText has us looking into µObjectifying this method
public string String()
{
string source = _source.String();
string target = _target.String();
int place = source.LastIndexOf(target, StringComparison.Ordinal);
return place == NotFound
? source
: source.Remove(place, target.Length).Insert(place, _replace.String());
}
This is doing a few operations on strings. We want to encapsulate that. These types of operations are going to be reusable classes. We do objects for these functions for more than just reuse. We do it for maintainability and testability.
If I want to test this class; I need to make sure that source has the target in it. This forces the test to KNOW about the implementation details of the class. It may seem small; but any change, like if the text is line split, will then break the test. The test broke because the internal implementation broke. This should be very much avoided.
There are 3 operations we want to pull out. LastIndexOf, Remove, and Insert. I've put each of these into a class with commit b799994.
Breaking each of these into their own µObject allows reusability. Any other particular textual manipulation will be able to be composed of these objects. If we need to build the same behavior elsewhere - It's available. No copy and pasting.
µObjects force code to have things only happen in a single place, in a single way.
The last TODO we have is in out Toppings. We have a for loop that would be more elegantly represented with LINQ.
public Money Cost(Money basePrice)
{
//TODO: Smelly - Looping vs LINQ
Money result = new Money(0);
foreach (ITopping topping in _toppings)
{
result += topping.Cost(basePrice);
}
return result;
}
Unfortunately Resharper isn't smart enough to see the Aggregate LINQ application here.
public Money Cost(Money basePrice) => _toppings.Aggregate(new Money(0), (m, t) => m + t.Cost(basePrice));
We love Resharper anyway. :)
Commit 80be756 sees this added to our code base.
That's all the TODO's that had been added. Do I think it's all "perfect"? Never.
I am feeling pretty good about this showing application of µObjects. There has been no new functionality for the Pizza Shop, but a lot of refactoring and extracting objects. We are making µObjects from the code. We're producing classes (such as those under /Library/Texts) that can server as a foundation for any other projects.