Refactor to MicroObjects - 03
More Action
It's time to get some more Rule Actions in place.
Let's give ourselves a reminder of what our code looks like
public Answer Transform(int source)
{
TurnCount turnCount = new IntTurnCount(source);
IRule multipleOfThreeAndFiveRule = new MultipleOfThreeAndFiveRule();
if (multipleOfThreeAndFiveRule.Matches(turnCount)) return new FizzBuzzAnswer();
IRule multipleOfFiveRule = new MultipleOfFiveRule();
if (multipleOfFiveRule.Matches(turnCount)) return new BuzzAnswer();
IRule multipleOfThreeRule = new MultipleOfThreeRule();
if (multipleOfThreeRule.Matches(turnCount)) return new FizzAnswer();
return new Default_RuleEvalAction().Act(turnCount);
}
And we have just implemented Default_RuleEvalAction. Our approach of "bottom up" creation of these actions will have us create the action for the MultipleOfThree_RuleEvalAction. This one will need to take in an IRuleEvalAction in the constructor so that the next action can be invoked.
public sealed class MultipleOfThree_RuleEvalAction : IRuleEvalAction
{
public MultipleOfThree_RuleEvalAction(IRuleEvalAction nextAction){
...
}
}
We'll continue to fill it out with the appropriate "Rule" and use constructor chaining
public sealed class MultipleOfThree_RuleEvalAction : IRuleEvalAction
{
private readonly IRule _rule;
private readonly IRuleEvalAction _nextAction;
public MultipleOfThree_RuleEvalAction(IRuleEvalAction nextAction) :this(new MultipleOfThreeRule(), nextAction){}
private MultipleOfThree_RuleEvalAction(IRule rule, IRuleEvalAction nextAction)
{
_rule = rule;
_nextAction = nextAction;
}
...
}
This allows us to have the object only interact with interfaces. Our Primary constructor, which is the one that assigns to class variables, has only interfaces (or abstract classes) as parameter types. This is how we maintain a decoupled code with concrete instantiation in the constructor. We only interact with interfaces (or abstract classes) in the class code.
For our Act implementation; if our rule matches, we want to return the answer, and if it does not match, return the results of the next action.
public Answer Act(TurnCount turnCount)
{
if (_rule.Matches(turnCount)) return new FizzAnswer();
return _nextAction.Act(turnCount);
}
There's a way to shrink this and minimize elements
public Answer Act(TurnCount turnCount)
{
return _rule.Matches(turnCount) ? new FizzAnswer() : _nextAction.Act(turnCount);
}
but I never feel like this expresses the intent as well. I love ternary operators, but it's not as informative when I read it.
The difference in information for me is that, do we care if the rule matches, or if it doesn't match? Which is the one this class cares about?
Even our if implementation above doesn't answer that question particularly well. It's just checking if the rule matches; but WHY? What does it mean for the rule to match?
We could just as easily write
public Answer Act(TurnCount turnCount)
{
if (!_rule.Matches(turnCount)) return _nextAction.Act(turnCount);
return new FizzAnswer();
}
Is either more informative? Does one tell us more than the other about the intent of this method?
Not really.
What about this?
public Answer Act(TurnCount turnCount)
{
if (ShouldHandle(turnCount)) return new FizzAnswer();
return _nextAction.Act(turnCount);
}
private bool ShouldHandle(TurnCount turnCount) => _rule.Matches(turnCount);
If we wanted to change the if
public Answer Act(TurnCount turnCount)
{
if (ShouldNotHandle(turnCount)) return _nextAction.Act(turnCount);
return new FizzAnswer();
}
private bool ShouldNotHandle(TurnCount turnCount) => !_rule.Matches(turnCount);
It's still very informative about what the branching is doing for us. We don't have to guess about how the rule match is being used, the additional method name tells us explicitly.
I like the first version. main aed4be2
To use this, we'll have to extract not invoke the Default rule in the code, but pass it in the constructor as mentiond.
public Answer Transform(int source)
{
TurnCount turnCount = new IntTurnCount(source);
IRule multipleOfThreeAndFiveRule = new MultipleOfThreeAndFiveRule();
if (multipleOfThreeAndFiveRule.Matches(turnCount)) return new FizzBuzzAnswer();
IRule multipleOfFiveRule = new MultipleOfFiveRule();
if (multipleOfFiveRule.Matches(turnCount)) return new BuzzAnswer();
return new MultipleOfThree_RuleEvalAction(new Default_RuleEvalAction()).Act(turnCount);
}
And since it's all green... we can keep it. main 93908d1
And we can move it to be with the other Actions.
I think it'sll be pretty clear how we can do this for the Multiple of 5 as well. main 4f209ae
public sealed class MultipleOfFive_RuleEvalAction : IRuleEvalAction
{
private readonly IRule _rule;
private readonly IRuleEvalAction _nextAction;
public MultipleOfFive_RuleEvalAction(IRuleEvalAction nextAction) : this(new MultipleOfFiveRule(), nextAction) { }
private MultipleOfFive_RuleEvalAction(IRule rule, IRuleEvalAction nextAction)
{
_rule = rule;
_nextAction = nextAction;
}
public Answer Act(TurnCount turnCount)
{
if (ShouldHandle(turnCount)) return new BuzzAnswer();
return _nextAction.Act(turnCount);
}
private bool ShouldHandle(TurnCount turnCount) => _rule.Matches(turnCount);
}
And... these are really similar. The only differences are the new MultipleOfFiveRule and new BuzzAnswer. Everything else is identical between the two rules so far. And I don't see any reason it'd change for Three and Five.
Let's put this class in place to make sure it works... because I had to re-write a little. I didn't change FizzAnswer to BuzzAnswer the first time... and got some red.
That's why we have tests!!!
With it updated and working properly, we're using our actions and staying green. main 5cc7fe4
public Answer Transform(int source)
{
TurnCount turnCount = new IntTurnCount(source);
IRule multipleOfThreeAndFiveRule = new MultipleOfThreeAndFiveRule();
if (multipleOfThreeAndFiveRule.Matches(turnCount)) return new FizzBuzzAnswer();
return new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new Default_RuleEvalAction())).Act(turnCount);
}
Extract common Action
We need to extract out the common action. The simplest way is to move the new BuzzAnswer to be a constructor argument.
public sealed class MultipleOfFive_RuleEvalAction : IRuleEvalAction
{
private readonly IRule _rule;
private readonly Answer _answer;
private readonly IRuleEvalAction _nextAction;
public MultipleOfFive_RuleEvalAction(IRuleEvalAction nextAction) : this(new MultipleOfFiveRule(), new BuzzAnswer(), nextAction) { }
private MultipleOfFive_RuleEvalAction(IRule rule, Answer answer, IRuleEvalAction nextAction)
{
_rule = rule;
_answer = answer;
_nextAction = nextAction;
}
public Answer Act(TurnCount turnCount)
{
if (ShouldHandle(turnCount)) return _answer;
return _nextAction.Act(turnCount);
}
private bool ShouldHandle(TurnCount turnCount) => _rule.Matches(turnCount);
}
and then use the tool to extract the super class
public class Base_RuleEvalAction : IRuleEvalAction
{
private readonly IRule _rule;
private readonly Answer _answer;
private readonly IRuleEvalAction _nextAction;
protected Base_RuleEvalAction(IRule rule, Answer answer, IRuleEvalAction nextAction)
{
_rule = rule;
_answer = answer;
_nextAction = nextAction;
}
public Answer Act(TurnCount turnCount)
{
if (ShouldHandle(turnCount)) return _answer;
return _nextAction.Act(turnCount);
}
private bool ShouldHandle(TurnCount turnCount) => _rule.Matches(turnCount);
}
public sealed class MultipleOfFive_RuleEvalAction : Base_RuleEvalAction
{
public MultipleOfFive_RuleEvalAction(IRuleEvalAction nextAction) : base(new MultipleOfFiveRule(), new BuzzAnswer(), nextAction) { }
}
I don't like the name Base_RuleEvalAction. Base never sits well with me. Until I have some more examples... it's hard to determine the uniqueness from a single example.
We can now shift MultipleOfThree_RuleEvalAction to use the abstract class. main f388e95
It looks surprisingly similar to our MultipleOfFive
public sealed class MultipleOfThree_RuleEvalAction : Base_RuleEvalAction
{
public MultipleOfThree_RuleEvalAction(IRuleEvalAction nextAction) : base(new MultipleOfThreeRule(), new FizzAnswer(), nextAction) { }
}
These classes do nothing. And I agree.
I refer to these as Knowledge Classes. They KNOW what is required to make the Multiple Of Three Rule work. The responsibility of this class is to encapsulate that knowledge. It is the SPOT for how to evaluate the Multiple of 3 rule. This class encapsulate the coupling between the MultipleOfThreeRule and FizzAnswer. Neither of those needs to know about the other. No other place in the system is burdened with that knowledge. It exists in one and only place - authoritatively. If the Multiple of 3 Rule ever provides a different answer, this one class, MultipleOfThree_RuleEvalAction is the only place in the code that has to change. No where else knows anything about it, and will remain entirely ignorant about it.
As it should be.
OK - Let's do 3 and 5.
public sealed class MultipleOfThreeAndFive_RuleEvalAction : Base_RuleEvalAction
{
public MultipleOfThreeAndFive_RuleEvalAction(IRuleEvalAction nextAction) : base(new MultipleOfThreeAndFiveRule(), new FizzBuzzAnswer(), nextAction) { }
}
That... was... easy. Like... Super simple. Pretty obvious the bits I need to modify.
Using it should be pretty simple... at least following the pattern we've had so far.
public Answer Transform(int source)
{
TurnCount turnCount = new IntTurnCount(source);
return new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new Default_RuleEvalAction()))).Act(turnCount);
}
And... Yeah. It works. main 25e815e
Test Updates
I'd like to get the new IntTurnCount(source) out of our method and into the tests.
Let's update a test to do that. main c32a267
[TestMethod]
public void GivenMultipleOf5ReturnsBuzz()
{
//ARRANGE
const int multiplicand = 5;
const string expected = "Buzz";
List<int> multiplierList = new() { 1, 2, 4 };
int randomIndex = Rand.Next(multiplierList.Count);
int multiplier = multiplierList.ElementAt(randomIndex);
TurnCount sourceInput = new IntTurnCount(multiplier * multiplicand);
//ACT
string actual = Transform(sourceInput);
//ASSERT
actual.Should().Be(expected);
}
It's a pretty simple change to update int sourceInput = multiplier * multiplicand; to be TurnCount sourceInput = new IntTurnCount(multiplier * multiplicand);
We haven't changed the implementation yet. We're creating the object, converting to an int, creating an object and passing it on. It's funny, but it ensures that everything's still working.
Since we can see this test continuing to pass, we can get the other tests in a single pass. main 980112c
The GivenInputReturnsStringOfInput had a little extra conversion to undergo to be able to encapsulate the input into a TurnCount.
KeyValuePair<int, string> keyValuePair = regressionValues.ElementAt(Rand.Next(0, regressionValues.Count));
TurnCount sourceInput = new IntTurnCount(keyValuePair.Key);
string expected = keyValuePair.Value;
Nothing crazy though.
This allows us to change the type of parameter on Transform to be TurnCount. main add3587
public Answer Transform(TurnCount source)
{
TurnCount turnCount = new IntTurnCount(source);
return new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new Default_RuleEvalAction()))).Act(turnCount);
}
We still haven't removed the instantiation in the method. The longer we can change very little the more confidence we can have that everything is working as we expect.
And now we can remove that line and update something so the param name matches what's passed into Act. I kept source.main fa38991
The First Object
Since we're in an object oriented system... seems really funny to have this monster new chain in a method. Should probably be in a class somewhere.
Since this is FizzBuzz... I think a FizzBuzz class is gonna be pretty good. It's great that our IDE and tools make extracting a method into a class really simple.
public class FizzBuzz
{
public Answer Transform(TurnCount source)
{
return new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new Default_RuleEvalAction()))).Act(source);
}
}
Not always the end result I want; but it's a class. main 2b432a8
We should fix up the test class to ... be more proper. I don't like how it creates a class field as a reference, but I understand why it does. main 7589dd2
An example of how our test now looks
[TestMethod]
public void GivenMultipleOf3And5ReturnsFizzBuzz()
{
//ARRANGE
const int multiplicand = 3 * 5;
const string expected = "FizzBuzz";
List<int> multiplierList = new() { 1, 2, 3 };
int randomIndex = Rand.Next(multiplierList.Count);
int multiplier = multiplierList.ElementAt(randomIndex);
TurnCount sourceInput = new IntTurnCount(multiplier * multiplicand);
FizzBuzz subject = new FizzBuzz();
//ACT
string actual = subject.Transform(sourceInput);
//ASSERT
actual.Should().Be(expected);
}
For the class itself; we need to move the object instantiation chain into the constructor.
First we extract it to an object in the method
public class FizzBuzz
{
public Answer Transform(TurnCount source)
{
IRuleEvalAction action = new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new Default_RuleEvalAction())));
return action.Act(source);
}
}
Then I extract it as a class field.
public class FizzBuzz
{
private IRuleEvalAction _action;
public FizzBuzz()
{
_action = new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new Default_RuleEvalAction())));
}
public Answer Transform(TurnCount source)
{
return _action.Act(source);
}
}
I haven't found a good way to move the instantiation into a chained constructor... so there's a little bit of manual work there.
public class FizzBuzz
{
private readonly IRuleEvalAction _action;
public FizzBuzz():this(
new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new Default_RuleEvalAction())))
){}
private FizzBuzz(IRuleEvalAction action) => _action = action;
public Answer Transform(TurnCount source) => _action.Act(source);
}
As I've mentioned else where, interfaces are important. we should create one for our FizzBuzz. It's gonna be a lame one that's just IFizzBuzz.
public interface IFizzBuzz
{
Answer Transform(TurnCount source);
}
public class FizzBuzz : IFizzBuzz
{
private readonly IRuleEvalAction _action;
public FizzBuzz():this(
new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new Default_RuleEvalAction())))
){}
private FizzBuzz(IRuleEvalAction action) => _action = action;
public Answer Transform(TurnCount source) => _action.Act(source);
}
Transform isn't really a good name any more. Since we're going pretty game themed for the naming, I think "Turn" is appropriate .main f69638d
It fits pretty well with the param being TurnCount.
The IDE is telling me that the interface method Turn is not required. Which means the use of Turn is being done off of the concrete instance. Since only tests do this, they should get updated to have the interface instead of concrete as the type. main ca9fadb
And of course, kick FizzBuzz and the interface into a separate file. main 7f806ec
Clean Up
It's always a good idea to go through the files and clean up whitespace and update any file/folder structure that may have gotten out of date. main 1a7d879
Before this ends, there needs to be a little discussion on the FizzBuzz class and ... Why?
public class FizzBuzz : IFizzBuzz
{
private readonly IRuleEvalAction _action;
public FizzBuzz():this(
new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new Default_RuleEvalAction())))
){}
private FizzBuzz(IRuleEvalAction action) => _action = action;
public Answer Transform(TurnCount source) => _action.Act(source);
}
This class is an example of a Chain of Responsibility pattern. We also call it a Chain of Strategies due to the fact it's a factory for a strategy pattern.
This class, FizzBuzz has a responsibility that none of the other classes have, temporal. The order of the IRuleEvalAction implementations MATTERS. We can't have MultipleOfThreeAndFive_RuleEvalAction anyhere except the first. The Default_RuleEvalAction must be the last. While we've used the type system of the language to enforce that it's not able to take something in it's constructor - Knowing it's last is important. ...
I don't like that it's Default_RuleEvalAction; it's now AsString_RuleEvalAction. main 1cbc63d
public class FizzBuzz : IFizzBuzz
{
private readonly IRuleEvalAction _action;
public FizzBuzz() : this(
new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new AsString_RuleEvalAction())))
)
{ }
private FizzBuzz(IRuleEvalAction action) => _action = action;
public Answer Turn(TurnCount source) => _action.Act(source);
}
As I was saying, knowing that the AsString is the last item is important. The functionality of FIzzBuzz is temporally coupled. Let's look at the very first FizzBuzz I put into this
public string FizzBuzz(int input){
if(input % (3 * 5) == 0) return "FizzBuzz";
if(input % 3 == 0) return "Fizz";
if(input % 5 == 0) return "Buzz";
return input.ToString();
}
if(input % (3 * 5) == 0) return "FizzBuzz"; MUST be first.
input.ToString() MUST be last.
The middle two can swap around with out impact, but other rules may make that not hold, the order of the rules are important. The order they get evaluated in determines if we get the right or wrong answer.
The FizzBuzz class seems largely like a pass through, and it is. That's probably why it seems like it. The non-pass through aspect is the knowledge of the temporal coupling. That's what the class knows. It knows how to properly evaluation the series of actions.
Further review of the code... There's one more small refactor to make. InputAsStringAnswer should be renamed to TurnCountAsStringAnswer.
Other than that ... Done.
What'd we do?
You've now see how we can take a 6 lines of code and make it into 161 lines of code.
While this may seem excessive - for a small solution; yes. Duh.
When you start to build larger systems, you have components you get to re-use. The CompoundRule starts to hint at the compositionability that this style yields.
What style? you ask? Well... that's MicroObjects. A style that takes well known industry practices and pushes them to the extreme.
I have a lot more about it here on my blog