What's an input?
Last time we transformed our returned values into Answers and used instances of those to be returned.
The other primitive we can see is the type of source and quite a few hard coded values.
Let's take a gander at the code again
public Answer Transform(int source)
{
if (0 == source % (3 * 5)) return new FizzBuzzAnswer();
if (0 == source % 5) return new BuzzAnswer();
if (0 == source % 3) return new FizzAnswer();
return new InputAsStringAnswer(source);
}
The source needs to be converted into some object. Some "Input"... Since we're going a bit closer to the game analogy for FizzBuzz... Current? ... Turn? Count? So many days worth of time spent trying to name the input to this method. Trying to create a class for the same thing... just end me now.
I'm going for the simple - Input. I don't think it's great. Definitely not. I hope I get some better names... someday...
One thing to consider is how broad the name applies. Much like Answer it can apply pretty broadly. And ... input sounds so ... uninformative. I ... just don't have good names for this. Something that popped into mind is ToAnswer. It's the thing to answer. While I feel it's more informative for what we're doing, it's pretty close to Answer and we want to avoid different things being very similar.
The 4 rules of simple design continue to be driving factors in these refactors.
TurnCount. I don't like either of those individually, but TurnCountseems pretty good. I'm gonna go with that.
OK, TurnCount. As an abstract class. What's our specific implementation? Well... ... It's an "Integer turn count" so... being very literal... IntTurnCount. I have a few conventions for this type of object that I've tried out; I think this is good though. It expresses intent pretty clearly. main 5ff50e2]
public abstract class TurnCount : ToSystemType<int>{}
public sealed class IntTurnCount : TurnCount
{
private readonly int _origin;
public IntTurnCount(int origin) => _origin = origin;
protected override int AsSystemType() => _origin;
}
This, as described by the name, takes an int to create the turn count. Currently we are allowing it to be converted to an int. This may not be the final result... I don't know. It's an adventure every time.
What the ToSystemType allows us to do, even if it goes away later, is wedge an object in that we can start to iterate on. main 28c1149
public Answer Transform(int source)
{
TurnCount turnCount = new IntTurnCount(source);
if (0 == turnCount % (3 * 5)) return new FizzBuzzAnswer();
if (0 == turnCount % 5) return new BuzzAnswer();
if (0 == turnCount % 3) return new FizzAnswer();
return new InputAsStringAnswer(source);
}
Since there's not a number of inputs, at this point, we can look at how we're using the turnCount value. The requirement, and if condition is "is it a multiple of".
We are extracting the raw data to ask it, "Are you a multiple of X?". We should move that behavior into the class by adding a method to represent that behavior.
public abstract class TurnCount : ToSystemType<int>
{
public abstract bool IsMultipleOf(int value);
}
In the implementation, we can just move the contents of the if condition main 455a633.
public override bool IsMultipleOf(int value)
{
return (0 == _origin % value);
}
We still have some ints floating around here, but we can simplify our main method main 50640a2.
public Answer Transform(int source)
{
TurnCount turnCount = new IntTurnCount(source);
if (turnCount.IsMultipleOf(3 * 5)) return new FizzBuzzAnswer();
if (turnCount.IsMultipleOf(5)) return new BuzzAnswer();
if (turnCount.IsMultipleOf(3)) return new FizzAnswer();
return new InputAsStringAnswer(source);
}
Maybe "simplify" is a bit much. At the least we have definitely improved the communication of what's happening. we don't need to know what % is to understand we're checking if the turnCount is a multiple of a value.
Here's a question I like to ask... which is why I'm including it here - Should the TurnCount know how to calculate a multiple of?
It seems a little bit outside it's scope of responsibility.
I definitely like it being removed from the Transform method. It doesn't belong there either. There should be a single point of truth for how to calculate if number is a multiple of another number.
Almost as if that belongs as a class.
As I type out
public sealed class IsMultipleOf
{
public bool Is(TurnCount turnCount)
{
}
}
I realize that this is a rule of the game. There can be other rules. One I've used to throw a wrench into training is if something contains the number it should behave as if it's a multiple of the number. An example is that "13" would return "Fizz"
This Concept of a rule should exist. It also helps define the method a bit better. I ended up with
public abstract class Rule
{
public abstract bool Matches(TurnCount turnCount);
}
public sealed class MultipleOfRule : Rule
{
public override bool Matches(TurnCount turnCount)
{
return false;
}
}
Clearly not completely implemented.
One thing glaringly missing is how we determine what TurnCount is a multiple of. Maybe it should be an abstract class itself. A further specification of a rule, but still not an 'end' rule.
We'd have to introduce something like a MultipleOfThreeRule to have a final rule
public abstract class Rule
{
public abstract bool Matches(TurnCount turnCount);
}
public abstract class MultipleOfRule : Rule
{
private readonly int _multipleOf;
protected MultipleOfRule(int multipleOf) => _multipleOf = multipleOf;
public override bool Matches(TurnCount turnCount) => turnCount % _multipleOf == 0;
}
public sealed class MultipleOfThreeRule : MultipleOfRule
{
public MultipleOfThreeRule():base(3){}
}
There's a few primitives floating around with our MultipleOf objects. But it's contained to those classes. Ones that explicitly need to deal with the primitives to make a determination. main b34fa5d
I'm spending quite a bit of time just starting at the code. Running ideas through my head. Clearly not all of them end up written here. I don't want to give the impression I can come up with these ideas on the fly. No no. 5 minutes, 15 minutes... just.. thinking. Oh yeah.
Sometimes I'll type out a snippet to see how the code likes it, but delete it quickly if it goes no where.
In case you're wondering; I've never used the Rule structure before. This is the first time I've gone with the "it's a game" concept. How we represent things in the code will drastically change how we think about how it can evolve.
Thinking of it as a game made the idea of "rules" so clear that it's a head smacking "DUH" moment for how to create a class to handle the mod operator.
While it's currently getting a little cluttered, using the new is pretty clean main 21e9c29.
public Answer Transform(int source)
{
TurnCount turnCount = new IntTurnCount(source);
Rule multipleOfThreeRule = new MultipleOfThreeRule();
if (turnCount.IsMultipleOf(3 * 5)) return new FizzBuzzAnswer();
if (turnCount.IsMultipleOf(5)) return new BuzzAnswer();
if (multipleOfThreeRule.Matches(turnCount)) return new FizzAnswer();
return new InputAsStringAnswer(source);
}
Let's kick the rules to their own file. main e2c5406
Early abstraction?
We have 2 abstract classes for a single check - That's a lot of abstraction for little functionality. Is it too much too soon?
It's a great question, and it definitely looks like it. When I started writing out this question, I wasn't sure. As I think ahead... I'm convincing myself a bit.
I want to represent the concepts that exist in the code as objects in the code. We have the concept of a rule of the game. We have a concept of a "Multiple Of Rule". These BOTH should be represented. Rule... probably better served as an interface than an abstract class... now that I see it.
Which... gets updated to follow C# convents as IRule.
public interface IRule
{
bool Matches(TurnCount turnCount);
}
public abstract class MultipleOfRule : IRule
{
private readonly int _multipleOf;
protected MultipleOfRule(int multipleOf) => _multipleOf = multipleOf;
public bool Matches(TurnCount turnCount) => turnCount % _multipleOf == 0;
}
That's the biggest changes with the type and name change. main 2876240
Let's create the rule for a multiple of 5, because I have questions about the other one.
public sealed class MultipleOfFiveRule : MultipleOfRule
{
public MultipleOfFiveRule() : base(5) { }
}
...
public Answer Transform(int source)
{
TurnCount turnCount = new IntTurnCount(source);
IRule multipleOfThreeRule = new MultipleOfThreeRule();
IRule multipleOfFiveRule = new MultipleOfFiveRule();
if (turnCount.IsMultipleOf(3 * 5)) return new FizzBuzzAnswer();
if (multipleOfFiveRule.Matches(turnCount)) return new BuzzAnswer();
if (multipleOfThreeRule.Matches(turnCount)) return new FizzAnswer();
return new InputAsStringAnswer(source);
}
When things are very similar, it's pretty quick to make these changes. main 8627c2d
Now, about the remaining 3*5. This could very easily be implemented as
public sealed class MultipleOfThreeAndFiveRule : MultipleOfRule
{
public MultipleOfThreeAndFiveRule() : base(3*5) { }
}
And it wouldn't be wrong.
I already have things that check we have a multiple of 3 and 5. What does it look like to re-use these objects. I'm all about re-use, so I love this idea.
public sealed class MultipleOfThreeAndFive : IRule
{
private readonly IRule[] _rules;
public MultipleOfThreeAndFive()
:this(new MultipleOfThreeRule(), new MultipleOfFiveRule()){}
private MultipleOfThreeAndFive(params IRule[] rules) => _rules = rules;
public bool Matches(TurnCount turnCount)
{
return _rules.All(it => it.Matches(turnCount));
}
}
We're implementing the IRule interface instead of the MultipleOfRule - which is OK, we don't need that functionality. Maybe that's not a good name to use, MultipleOfRule It's accurate, but isn't for all multiples of, clearly.
I'll put that though on the back burner while we look at this method a little closer. Before we do... Sorry, jumping all around - Let's use it!
We don't want to do a bunch of refactoring and changing if we've messed up this early on. Once we're using it, and the tests continue to pass, we'll know that we're not breaking anything with future changes.
Since we continue to be green; we can now go ... not to our back burners. we can clean up the IsMultipleOf method from TurnCount.
Returning to very simple objects. main 3c54254
public abstract class TurnCount : ToSystemType<int>{}
public sealed class IntTurnCount : TurnCount
{
private readonly int _origin;
public IntTurnCount(int origin) => _origin = origin;
protected override int AsSystemType() => _origin;
}
I like that much better.
Now, back to looking at MultipleOfThreeAndFive
public sealed class MultipleOfThreeAndFiveRule : IRule
{
private readonly IRule[] _rules;
public MultipleOfThreeAndFiveRule()
:this(new MultipleOfThreeRule(), new MultipleOfFiveRule()){}
private MultipleOfThreeAndFiveRule(params IRule[] rules) => _rules = rules;
public bool Matches(TurnCount turnCount)
{
return _rules.All(it => it.Matches(turnCount));
}
}
I like to use params to keep the primary constructor simple. It's an easy way to deal with multiple common types. It made me realize that this is a compound rule. Which... DUH... but it's much more obvious in this form than if I have separate variables for MultipleOfThreeRule and MultipleOfFiveRule. Like this, it's clear that a compound rule is an extractable concept from this class.
public abstract class CompoundRule : IRule
{
private readonly IRule[] _rules;
protected CompoundRule(params IRule[] rules) => _rules = rules;
public bool Matches(TurnCount turnCount) => _rules.All(it => it.Matches(turnCount));
}
public sealed class MultipleOfThreeAndFiveRule : CompoundRule
{
public MultipleOfThreeAndFiveRule()
: base(new MultipleOfThreeRule(), new MultipleOfFiveRule()) { }
}
I really like this. main 5b35e95
Now I can build up combination of any number of rules that get added! With minimal effort. If it's a rule, it can be part of a compound rule.
This is code re-use.
Back to ... it
public Answer Transform(int source)
{
TurnCount turnCount = new IntTurnCount(source);
IRule multipleOfThreeRule = new MultipleOfThreeRule();
IRule multipleOfFiveRule = new MultipleOfFiveRule();
IRule multipleOfThreeAndFiveRule = new MultipleOfThreeAndFiveRule();
if (multipleOfThreeAndFiveRule.Matches(turnCount)) return new FizzBuzzAnswer();
if (multipleOfFiveRule.Matches(turnCount)) return new BuzzAnswer();
if (multipleOfThreeRule.Matches(turnCount)) return new FizzAnswer();
return new InputAsStringAnswer(source);
}
This is getting a little out of control now.
Let's re-organize it.
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 InputAsStringAnswer(source);
}
main c255a0c We now have some very discrete steps that either do something, or hand it off to the next step.
That sounds a lot like a decorator, or chain of responsibility. I tend towards chain of responsibility since we're not actually "decorating". None of these are option. We're not building up attributes of a coffee drink (I remember you Head First Design Patterns).
To put these into discrete steps, we need an interface that they will each implement, and ones that aren't the last, can take in the constructor.
I'm a sucker for tradition... in code... for things I like doing... OK. I have my preferred naming! I like to suffix the steps as "Action"
These are a "Rule Evaluation Action". Action is PURELY a artifact of naming conventions the teams used to make identification of classes easy. I value that, so I'm sticking with it.
This is my first, quick and dirty interface
public interface IRuleEvalAction
{
public Answer Eval(TurnCount turnCount);
}
We'll Eval our TurnCount and give back an Answer. Part of the action suffix is that the method was named Act. It allowed consistency across multiple chains of responsibility. We didn't use a shared interface to enforce it, but it made like A LOT easier when all of the same type of things used the same method name.
poof
public interface IRuleEvalAction
{
public Answer Act(TurnCount turnCount);
}
I think the clarity it provided by doing that is something worth keeping.
Implementing the Default case made me realize a refactor we didn't do.
Let's get it in place first.
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 InputAsStringAnswer(source);
}
Going back to our Transform method, our default case is taking in the source in it's constructor. It should take in the turnCount. It should have much earlier in this refactor, but that's ok.
public sealed class InputAsStringAnswer : Answer
{
private readonly TurnCount _input;
public InputAsStringAnswer(TurnCount input) => _input = input;
protected override string AsSystemType() => ((int)_input).ToString();
}
...
return new InputAsStringAnswer(turnCount);
We have a hard cast of our TurnCount _input to be able to turn it to a string, but that's exactly what we want to see. We want to make it clear that we are forcing the primitive type out of the object and interacting with it.
To get the string version of our TurnCount is just in this one place. A Single Point Of Truth.
Back to the Default Action
Now that we've updated our InputAsStringAnswer to take a TurnCount, we can make our Default Action look like main 482ed93
public sealed class Default_RuleEvalAction : IRuleEvalAction
{
public Answer Act(TurnCount turnCount)
{
return new InputAsStringAnswer(turnCount);
}
}
TECHNICALLY we could have done this without modifying the constructor param type, as TurnCount will auto cast to an int. I prefer to make the change early... ya never know what you'll forget about.
We can now replace the last answer being returned with an instantiation and invocation of the Act method.
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);
}
With everything staying green, it's a good time to move the RuleEvalAction items to a separate file.
Next Time
We're pretty far along. Let's save the other RuleEvalAction implementations for next time.
I do want to address a convention breaker - Default_RuleEvalAction. What's an underscore doing in the name?! That's MADNESS!
And I railed against it. Said we shouldn't.
I'm the technical lead and there's no way we're doing that!
No, I never said that. That's being a jerk. I said
I hate it, but if you say there's value, let's try it. Re-names are easy
And... it's so nice. It takes a lot for me to agree to a violation of norms... but this _ underscore as part of the name of the actions in a chain of responsibility make it SO MUCH easier to visually parse the information. The entire teams could parse and understand the file names far more effectively by introducing this underscore. Definitely something to consider using when applying this pattern.
I don't typically use it for small or one off projects, but if it's anything important, and me sharing how to do this well is important, it's definite must. At least to try. Like I said to the team at the time, renaming is easy.
When we come back, we'll add the other Actions.