Not Done Yet!
Jim Speaker, someone who strongly applies most, if not all, of the practices I advocate, posted a version of FizzBuzz that demonstrates a lot of these practices.
I liked most of it. I've developed some other techniques since we worked on the same team. Through the previous posts you saw my approach to FizzBuzz and TDD, and these extra techniques I use; though you may not recognize them as such. In the transformation I tried to share what I look at when I'm applying the MicroObject practices.
In the comments of the linked in post, Leonard Sperry has some questions and critiques of the style. The biggest is that it's not open for extension. Code that's not open for extension typically takes a paradigm shift to extend.
I fully agree.
Except on "not open for extension". I don't agree. I don't really disagree, but I think it changes the concept of how we do extension.
Most of the practices I advocate are focused on internal to your code. It's why I abstract 3rd party code. It's why I have an abstracted way to convert from object to primitives.
Someone else extending the code isn't what these practices are meant for. By 'someone else' I mean someone external to the codebase, not another developer on the team.
Exploration
This is about exploring how we can improve the code. Find new patterns to solve problems. This is an exercise in problem solving. We get to use FizzBuzz for this problem solving.
We know and understand FizzBuzz. We KNOW if we have the right output, increasingly so with the tests. We can explore how to make the temporal coupling of the FizzBuzz rules more flexible. It'll look different, a paradigm shift, but that's fine. Multiple paradigms to solve the problem only gets us so we can make more flexible code.
As part of the fact this is exploration - It's not going to be micro commits. I'll often write 50-100 lines to test an idea and if I like it... I have the code. If I don't like it, I hit delete. (usually through git reset --hard).
This series has shown how to "TDD Like You Mean It" and "Refactor to MicroObjects" transformation, this post (series?) will be about exploring how we can get a pattern I don't have an answer for.
It's "Explore to a Pattern" time!
Different Rules for Different Code
I often say that tests get slightly different rules. Duplication and utility classes using reflection aren't unknown in my tests - Though I'll damn sure stamp it out in code.
Tests get different rules.
Code that someone else consumes, also gets different rules. I'll violate a large number of my code practices at the boundary of the Library.
Building a NuGet package type of system, oh yeah, vastly different rules. I do NOT want to force other developers into the mass of practices I use. I want to provide a clean, concise interface that they can provide configuration for, when needed.
I won't go into extension in that situation; It doesn't apply to this style. System boundaries play by different rules. External facing APIs play by different rules.
APIs in particular should be constructed with "Ease of Use" as one of the highest principles. While ease of use is also important for the internal apis, it's not that high. Maintainability, extensibility, flexibility... there's a whole slew of -ilities that we focus on; Usability is one of them; but not the biggest. I look at Maintainability as the most important for the internal code of a system.
Extensibility
The question Leonard asked around extensibility was if we wanted to add a new rule.
It is certainly an interesting way of encapsulating the requirements. However, I do have some questions for you to ponder. Do you think this solution is open for extension? Imagine the product owner came and said "In addition to our Fizz and Buzz offering, we will now be offering Bang with the number 7." How much boiler plate would you need to add? Aside from FizzBuzzBang, could you offer FizzBang or BuzzBang? Leonard Sperry
Jim's answer to this was to update his existing chain of responsibility from
public FizzBuzzString(int value) :
this(new FizzBuzzStrategy(value,
new BuzzBangStrategy(value,
new BuzzStrategy(value,
new FizzBangStrategy(value,
new FizzStrategy(value,
new BangStrategy(value,
new StringStrategy(value)))))))) { }
to
public FizzBuzzString(int value) :
this(new FizzBuzzBangStrategy(value,
new FizzBuzzStrategy(value,
new BuzzBangStrategy(value,
new BuzzStrategy(value,
new FizzBangStrategy(value,
new FizzStrategy(value,
new BangStrategy(value,
new StringStrategy(value))))))))) { }
Adding in the 4 new strategies.
That's not what I'd do.
But when is it ever. Even if it is what I'd do, I'd probably want to try something different.
I'm writing this because I tried to type up a response on LinkedIn... and it felt hollow. I couldn't come up with an answer to how someone else could extend and control the rules from outside of the system. If I wanted it to be extendable for an external consumer.
I didn't like my very weak, "it doesn't matter here" answer that I was coming up with.
It Matters. No, not here, but it's a space I want to figure out. I don't like that I don't have an answer. I have an idea now, and I'll get to that.
Let's first look at how I'd implement the addition of FizzBuzzBang, FizzBang, and BuzzBang.
Bang - Not just my energy drink
What' the order? With just 3 and 5 it's pretty clear.
I think we can agree 3*5*7 is first.
Next, we have a three options
- 3*5
- 3*7
- 5*7
There are 6 ways to arrange these Does an order of these 3 matter? ... Maybe... Time to use EXCEL. Quick Maths!
No. Does not matter. Any time one 15, 21, or 35 is evenly divisible by another value, it's also divisible by 3*5*7. Which... now that I think about it a little more... yeah... obvious.
N/(x*3*y*5*z*7) where x,y,z are either 1 or 2. Has to be since 3*5*3*7, 2*3*1*5*1*7.
OK.. Got it. So, no. The order of these three do not matter.
Next up we'll have 3, 5, 7. Which, order also doesn't matter.
Interestingly we're evolving to "groups" which weren't obvious in the smaller example.
We have N-Factor groups. The order internal to this doesn't matter, but the order of the factor groups do.
How I'd build this into my system default, with just adding 7.
I'd build a new class. I currently have FizzBuzz and I'd add a FizzBuzzBang class.
public sealed 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);
}
public sealed class FizzBuzzBang : IFizzBuzz
{
private readonly IRuleEvalAction _action;
public FizzBuzzBang() : this(
new MultipleOfThreeAndFiveAndSeven_RuleEvalAction(
new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfThreeAndSeven_RuleEvalAction(
new MultipleOfFiveAndSeven_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new MultipleOfSeven_RuleEvalAction(
new AsString_RuleEvalAction())))
)
{ }
private FizzBuzzBang(IRuleEvalAction action) => _action = action;
public Answer Turn(TurnCount source) => _action.Act(source);
}
We can question the accuracy of IFizzBuzz but I never liked that anyway. I think IRuleEvalAction is better. As I sadly forgot when I was implementing this... The top of the chain implements the same interface as the rest of the chain.
The new class would look like
public sealed class FizzBuzzBang : IRuleEvalAction
{
private readonly IRuleEvalAction _action;
public FizzBuzzBang() : this(
new MultipleOfThreeAndFiveAndSeven_RuleEvalAction(
new MultipleOfThreeAndFive_RuleEvalAction(
new MultipleOfThreeAndSeven_RuleEvalAction(
new MultipleOfFiveAndSeven_RuleEvalAction(
new MultipleOfFive_RuleEvalAction(
new MultipleOfThree_RuleEvalAction(
new MultipleOfSeven_RuleEvalAction(
new AsString_RuleEvalAction())))
)
{ }
private FizzBuzzBang(IRuleEvalAction action) => _action = action;
public Answer Act(TurnCount source) => _action.Act(source);
}
I'd probably replace the excessive boiler plate of the public Answer Act(TurnCount source) => _action.Act(source); into an abstract base class. Making FizzBuzz and FizzBuzzBang both Knowledge Classes.
External?
How can this be configured for more extensibility with external consumers? It does require a bit of a paradigm shift. How you let external consumers interact with the code vs internal consumers IS a paradigm shift.
Under just the addition of the multiple of 7... I see something forming. It also, interesting, has highlighted that I don't have a Rule for the default case, just an answer.
That's a pretty easy fix, though it'd have to be put together.
The way I can see this being extensible is by the number of factors. 3, 2, 1, 0. These are grouped together and their order doesn't matter. If we provide, and can compare, the number of factors then we could bucket them like that.
What about if we don't look at it like that. What if we change the structure to just provide the individual factors? 3, 5, and 7. Have a system in place that can execute against each of those.
Some kinda "Pair" to associate 3 with Fizz and 5 with Buzz, etc.. That will fail if we switch from 15 being FizzBuzz to being FIZZYBUZZY. Adding that in would be... harder. I don't want to treat a multiple of 3 equivalent to including Fizz in the string. They can change independently. We need each rule to be it's own rule.
The organization says we should have factor groups; as the new rule is showing us. I doubt this will hold for all other rules we can include, since some aren't multiples, such as the ToString case.
We strongly avoid Gold-Plating as it leads to nothing but pain. Build for what you know, and when you know more, build more.
Approach to Factor groups
I think my approach to this will be a collection of IRuleEvalActions that can be looped through according to the factors. This will change the constructor.
It is a paradigm shift to enable the flexible for someone outside the code. And that's ok. Changing from internal to external consumers SHOULD be a shift. If it's not... your consumers are either going to be confused or internally you lack flexibility.
That doesn't mean the structures we find for external use are not also good for internal, just different pressures driving different designs.
Quick Fixes
While I'm not doing micro-commits, I do want to get some pieces out of the way before I start doing bigger commits.
As I mentioned that FizzBuzz should implement the IRuleEvalAction like the steps it takes. This allows it to be flexibile and includable in a bigger flow without silly hacks. I'm going to leave it named as FizzBuzz for now. Not thrilled with it, but also not thrilled with FizzzBuzz_RuleEvalAction. extendIt 7cf7219c
I started to go down the path of using a AsStringRule for use in the AsString_RuleEvalAction and it did not want to be there. I'm gonna listen to the code and not have it in.
I highly recommend you do it as an exercise if you're following along.
I'm not a huge fan of the name TurnCount so I'm going to change it to ... TurnNumber, GameInput, TurnInput? I like TurnInput vs TurnCount. "Input" allows flexibility where as count seems pretty tied to the number of turns that have occurred.
This change has a pretty broad impact. extendIt 6ba91b61
Make it Shift
OK, let's get to this paradigm shift for adding rules to FizzBuzz.
We need our rules to be updated to have a "Factors" value which can be used as a comparison factor. The only way to be able to compare all rules is with the comparison in IRule... which means it needs to be a class (though abstract) so it can churn against itself without making the factor value public. extendIt 6dc6080
Then we need to provide the factor value for Rule. I'm going to do this via the constructor. I have a strong aversion to abstract methods when composition can be used, even in classes currently extending.
I'm sure I have it in a few other rants, but inheritance is coupling between the base class and derived class. Changes in the base MUST be handled by the derived. If something becomes public, there's no control in the derived classes. Exposure is uncontrolled, and that's risky for the object. If we follow good practices, it can work, but sometimes a one-off has to be added to a base class. I've seen that too much.
If it can be a composition it should be.
Now that we've shifted from an interface to an abstract class, we can actually re-visit this method. And we will... after we do the factor component for comparison.
Since we favor composition, let's just pass it to the constructor of our new Rule class
public abstract class Rule
{
private readonly int _factors;
protected Rule(int factors) => _factors = factors;
public abstract bool Matches(TurnInput turnInput);
}
and update the Rules to provide a value that matches their factors.
public abstract class MultipleOfRule : Rule
{ ...
protected MultipleOfRule(int multipleOf) : base(1) => _multipleOf = multipleOf;
...
}
...
public abstract class CompoundRule : Rule
{
...
protected CompoundRule(params Rule[] rules) : base(rules.Length) => _rules = rules;
...
}
And that's it. extendIt a721a89
Of course, that's it for adding, but we haven't gotten comparrison in place. Fortunately C# has a simple way to do this, IComarable<T>. The tools generate the code for me, I just had to type out : IComaprable<Rule> and I get the implementation generated for me. Of course, we have a simple comparrison, it may not always generate as needed. shakes fist at int
public abstract class Rule : IComparable<Rule>
{ ...
public int CompareTo(Rule? other)
{
if (ReferenceEquals(this, other)) return 0;
if (ReferenceEquals(null, other)) return 1;
return _factors.CompareTo(other._factors);
}
}
We now have to consider how to change FizzBuzz so the rules can use the comparable. The simples is ... a collection... which I mentioned above. Excellent. At least I'm consistent in my thinking over short periods of time.
I'd consider a change from a the nested structure to a flat collection as a pretty risky change. Which is why I make new objects. I've always got the good state available.
As we switch to a collection, I think there's some constructs that will have to be in place temporarily, and maybe permanently, to keep the existing working, while the new style takes shape.
One of these is that, currently, each Action is expecting the next action in it's constructor. We need a temp "non-answer" Action to provide them.
Which then forces us to need to evaluate the result to know if we should return it, or call the next Action. This is VERY similar to our Matches method. Maybe that should get exposed?
I don't like exposing both a Matches and Answer method. They BOTH have to be called. I don't like making the client do that extra work. Also don't like having to check the response every time.
I have a couple ideas.
- Checking
- Visitor Pattern
- Collector? Pattern
- Collector with "HasAnswer"
Those are my initial ideas. I think checking is the simplest and least complex, so lets do that and see how it looks. Unfortunately we're dealing with null, but that should be correctable later.
We can hack in something like this
public sealed class FizzBuzzAsCollection : IRuleEvalAction
{
public Answer Act(TurnInput turnInput)
{
var nullAnswer = new NullAnswer_RuleEvalAction();
List<IRuleEvalAction> list = new List<IRuleEvalAction>
{
new MultipleOfThreeAndFive_RuleEvalAction(nullAnswer),
new MultipleOfFive_RuleEvalAction(nullAnswer),
new MultipleOfThree_RuleEvalAction(nullAnswer),
new AsString_RuleEvalAction()
};
list.Sort();
foreach (IRuleEvalAction action in list)
{
Answer result;
if((result = action.Act(turnInput)) != null) return result;
}
throw new Exception("No Answer Provided");
}
}
But... uhhh... I made a small mistake on where I put the factor.
I hacked in usage of FizzBuzzAsCollection into FizzBuzz
public Answer Act(TurnInput turnInput) => new FizzBuzzAsCollection().Act(turnInput);
And all tests fail.
Test method TddLikeYouMeanIt.FizzBuzzTests.GivenMultipleOf5ReturnsBuzz threw exception:
System.InvalidOperationException: Failed to compare two elements in the array. ---> System.ArgumentException: At least one object must implement IComparable.
The IRuleEvalAction isn't where the factor lives. I think we'll be OK if we add IComparable to the IRuleEvalAction. It'll force some interesting decisions.
public interface IRuleEvalAction : IComparable<IRuleEvalAction>
I'm keeping it an interface to force some decisions.
As an example
public sealed class AsString_RuleEvalAction : IRuleEvalAction
{
public Answer Act(TurnInput turnInput) => new TurnCountAsStringAnswer(turnInput);
}
This class must now implement the comparable. Which allows us to ensure it's sorted after everything else, without any forcing.
public sealed class AsString_RuleEvalAction : IRuleEvalAction
{
public Answer Act(TurnInput turnInput) => new TurnCountAsStringAnswer(turnInput);
public int CompareTo(IRuleEvalAction? other) => -1;
}
Just always return -1. This way it's ALWAYS the last item. ... Or should that be 1 to always be bigger... Crap... Whelp... I've got some tests that'll tell me.
Going through the fuller implementation... I don't like how that looks. Feels wrong. Doesn't fit smoothly into the code. I'll roll it back and try something else.
Since I'm spending a bit of time starting at the code, it looks like Base_RuleEvalAction should currently be called MultipleOf_RuleEvalAction. Only MultipleOf* implement it. extendIt 5c7f6c44
An easy change that helps keeps thing better expressing their place in the code.
Shared behavior is creeping in. I definitely try to avoid it, but I've found interfaces that want to churn against themselves need to be a class so they can hold the churn value. In this case the Rule. We could definitely move the Factors value into the RuleEvalAction, but it's a piece of data that's tightly coupled to the rule, and we'd be pulling it out. It's better to leave it where it's tightly coupled.
Since we do this, things like our CompundRule can extract the value, and it doesn't have to be hard coded anywhere.
The change of FizzBuzz to use the IRuleEvalAction interface, while I believe it's correct, is giving some grief. Our paradigm shift doesn't always have a smooth transition.
Hmmm.... I feel like I may need to not try to do a slow progression. It's a new thing... just implement it fresh. If there's similarities that can be combind, we'll see it then. If not... it is a different paradigm.
It's late enough that I'll be doing that tomorrow... Or next time. We'll see what I do.
The Next Night... with insomnia
After being unable to sleep for a few hours, I got up to and have hacked together the idea I was going for. extendIt 1fd5beb
I call this hacked together as there's a few bits that feel very off, but the idea is here. It would definitely need some tweaks to correctly support the desrired external consumer model.
But let's get into it
Extendable?
I have a FizzBuzzExternal to be the main class, mostly to distinguish from FizzBuzz.
public sealed class FizzBuzzExternal : IFuzzBuzzExternal
{
private readonly IRuleEvalCollection _ruleCollection;
public FizzBuzzExternal(IRuleEvalCollection ruleCollection) => _ruleCollection = ruleCollection;
public Answer Eval(TurnInput turnInput)
{
foreach (IRuleEval rule in _ruleCollection)
{
if (rule.Matches(turnInput)) return rule.Response(turnInput);
}
return _ruleCollection.DefaultAnswer(turnInput);
}
}
All this knows how to do is evaluate the rules and return the appropriate one, as well as when to grab the defaul. I could make default a rule, sure... didn't.
public class AsStringFactorRuleCollection : IRuleEvalCollection, IRuleEvalCollector
{
private readonly IRuleEval _defaultRuleEval;
private readonly List<ComparableRuleEval> _collection = new();
public AsStringFactorRuleCollection() : this(new AsStringRuleEval()) { }
private AsStringFactorRuleCollection(IRuleEval defaultRuleEval) => _defaultRuleEval = defaultRuleEval;
public Answer DefaultAnswer(TurnInput turnInput) => _defaultRuleEval.Response(turnInput);
public IEnumerator<IRuleEval> GetEnumerator() => _collection.GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
public void Add(ComparableRuleEval ruleEval)
{
_collection.Add(ruleEval);
_collection.Sort((x, y) => y.CompareTo(x));
}
public void Clear() => _collection.Clear();
}
I've called this the AsStringFactorRuleCollection ... The AsString part defines the default answer. This is one of the things I'm not a huge fan of. It feels clunky. Since everything else is being added, why not add the default rule eval things.
Why even have it an eval? Just provide the answer. ... Oh well, hacked together, doing stuff.
The addition of ComparableRuleEvals is where the sorting happens. If this was long running, set up is a great place to put the cost of sorting. It can always be delayed/cached, but I just did it here.
What I've done is just invert the sorting. Another way would be (x, y) => -x.CompareTo(y). Feel free to use whatever.
public abstract class ComparableRuleEval : IRuleEval, IComparable<ComparableRuleEval>
{
private readonly Rule _rule;
private readonly Answer _answer;
protected ComparableRuleEval(Rule rule, Answer answer)
{
_rule = rule;
_answer = answer;
}
public bool Matches(TurnInput turnInput) => _rule.Matches(turnInput);
public Answer Response(TurnInput turnInput) => _answer; //It's a `getter` :(
public int CompareTo(ComparableRuleEval? other) => this._rule.CompareTo(other?._rule);
}
public sealed class MultipleOfThreeRuleEval : ComparableRuleEval
{
public MultipleOfThreeRuleEval() : base(new MultipleOfThreeRule(), new FizzAnswer()) { }
}
public sealed class MultipleOfFiveRuleEval : ComparableRuleEval
{
public MultipleOfFiveRuleEval() : base(new MultipleOfFiveRule(), new BuzzAnswer()) { }
}
public sealed class MultipleOfThreeAndFiveRuleEval : ComparableRuleEval
{
public MultipleOfThreeAndFiveRuleEval() : base(new MultipleOfThreeAndFiveRule(), new FizzBuzzAnswer()) { }
The comparable rules look REALLY similar. Unfortunately a getter has snuck in. I have ways to remove that, like a lambda, but it's just an abstraction on top of the same thing. I'd rather be able to easily identify the smell than have it hidden behind an abstraction.
A lambda could be used to always return a new one, sure. There's a few ways to handle it. I don't want to bother with solving that right now, not my target, so... Nope; have a getter. It'll get cleaned up later.
The rules are still really simple.
BIG FINISH
Here's a test for FizzBuzzExternal
[TestMethod]
public void External_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);
TurnInput sourceInput = new IntTurnInput(multiplier * multiplicand);
AsStringFactorRuleCollection ruleCollection = new()
{
new MultipleOfThreeRuleEval(),
new MultipleOfFiveRuleEval(),
new MultipleOfThreeAndFiveRuleEval()
};
IFuzzBuzzExternal subject = new FizzBuzzExternal(ruleCollection);
//ACT
string actual = subject.Eval(sourceInput);
//ASSERT
actual.Should().Be(expected);
}
We can see the build up of the rules. To add the rules for 7, we need to create the rules.
All of the rules are added here extendIt 2b34dbd.
public sealed class MultipleOfSevenRule : MultipleOfRule
{
public MultipleOfSevenRule() : base(7) { }
}
public sealed class MultipleOfThreeAndSevenRule : CompoundRule
{
public MultipleOfThreeAndSevenRule()
: base(new MultipleOfThreeRule(), new MultipleOfSevenRule()) { }
}
public sealed class MultipleOfFiveAndSevenRule : CompoundRule
{
public MultipleOfFiveAndSevenRule()
: base(new MultipleOfFiveRule(), new MultipleOfSevenRule()) { }
}
public sealed class MultipleOfThreeAndFiveAndSevenRule : CompoundRule
{
public MultipleOfThreeAndFiveAndSevenRule()
: base(new MultipleOfThreeRule(), new MultipleOfFiveRule(), new MultipleOfSevenRule()) { }
}
public sealed class BangAnswer : Answer
{
protected override string AsSystemType() => "Bang";
}
public sealed class FizzBangAnswer : Answer
{
protected override string AsSystemType() => "FizzBang";
}
public sealed class BuzzBangAnswer : Answer
{
protected override string AsSystemType() => "BuzzBang";
}
public sealed class FizzBuzzBangAnswer : Answer
{
protected override string AsSystemType() => "FizzBuzzBang";
}
public sealed class MultipleOfSevenRuleEval : ComparableRuleEval
{
public MultipleOfSevenRuleEval() : base(new MultipleOfSevenRule(), new BangAnswer()) { }
}
public sealed class MultipleOfThreeAndSevenRuleEval : ComparableRuleEval
{
public MultipleOfThreeAndSevenRuleEval() : base(new MultipleOfThreeAndSevenRule(), new FizzBangAnswer()) { }
}
public sealed class MultipleOfFiveAndSevenRuleEval : ComparableRuleEval
{
public MultipleOfFiveAndSevenRuleEval() : base(new MultipleOfFiveAndSevenRule(), new BuzzBangAnswer()) { }
}
public sealed class MultipleOfThreeAndFiveAndSevenRuleEval : ComparableRuleEval
{
public MultipleOfThreeAndFiveAndSevenRuleEval() : base(new MultipleOfThreeAndFiveAndSevenRule(), new FizzBuzzBangAnswer()) { }
}
The only bit that might sit a little unwell with me is the additional Answer classes. I think the reason these sit unwell is that they are full on classes, when they... don't really need to be.. These are constants. one instance of FizzAnswer should be referentially equal to another instance of FizzAnswer. We need a single instance of a FizzAnswer across the entire solution.
I'm not implementing this right now, but here's the rough sketch of how I'd transform the existing code
public abstract class Answer : ToSystemType<string> { }
public sealed class FizzBuzzAnswer : Answer
{
protected override string AsSystemType() => "FizzBuzz";
}
into static values for each
public abstract class Answer : ToSystemType<string>
{
public static Answer FizzBuzz = new AnswerOf("FizzBuzz");
private sealed class AnswerOf : Answer
{
private readonly string _answer;
public AnswerOf(string answer) => _answer = answer;
protected override string AsSystemType() => _answer;
}
}
For better extensibility, maybe AnswerOf gets moved outside the
All this really does is reduce the boiler plate (which is nice). What I don't like about it is the challenge to implementing more answers... This class isn't extendable like that. We're looking for static fields... So... not great.
Another way that would do away with the answer specific classes entirely
public abstract class Answer : ToSystemType<string>, IComparable<Answer>
{
public int CompareTo(Answer? other) => string.Compare(this, other!, StringComparison.Ordinal);
}
public sealed class AnswerOf : Answer
{
private readonly string _answer;
public AnswerOf(string answer) => _answer = answer;
protected override string AsSystemType() => _answer;
}
public sealed class MultipleOfSevenRuleEval : ComparableRuleEval
{
public MultipleOfSevenRuleEval() : base(new MultipleOfThreeRule(), new AnswerOf("Bang")) { }
}
This is ... better. What I don't like about it is that the MultipleOfSevenRuleEval has gone beyond just knowing what classes need to be used to knowing the MultipleOfSeven Specific Answer. That's more knowledge than the class should have. It's... letting this class have to change for multiple reasons.
Even though it's a bit of boilerplate... I do like the answers as specific classes. We have a representation of the concept, "a Bang answer" as a class. That's the driving philosophy... I feel best when it's strongly followed.
FLAKEY TEST
OH, OH! OH!! I got a flakey test!!! The 5 and 7 test is flakey!!! The error message SUCKS! I can't figure out what value ran.
Expected actual to be "BuzzBang" with a length of 8, but "FizzBuzzBang" has a length of 12, differs near "Fiz" (index 0).
Let's update the assert to include improved messaging
actual.Should().Be(expected, $"Ran with value of [5*7*{multiplier}={multiplicand * multiplier}]");
which gives us
Expected actual to be "BuzzBang" with a length of 8 because Ran with value of [5*7*3=105], but "FizzBuzzBang" has a length of 12, differs near "Fiz" (index 0).
HAHAHA... I'm a dumb.
3 can't be used as a multiplier! Switching that to good old trusty 4.
Since this failure showed a weakness in the reporting, we'll migrate the string to the other methods. I'm going to collapse 5*7 into multiplicand so it's a common message across all tests. Maybe...
Expected actual to be "BuzzBang" with a length of 8 because Ran with value of [35*3=105], but "FizzBuzzBang" has a length of 12, differs near "Fiz" (index 0).
I don't think that's as good. the fact it was 5 * 7 * 3 made it REALLY obvious. Fine, I'll leave it and update per test.
I called it out earlier in the series... 3 is a risky multiplier. I'm going to change all 3 factors to 4 as well as adding the improved messaging.
These tests are added here extendIt 99bc2ff.
Extendable
This sample isn't something I'm happy with. It wasn't meant to be either.
This is an example of shifting the paradigm to create extendibility w/o modifying the temporal coupling class. I don't think it could have been doable until the addition of the 7 and seeing the patterns of groups where order doesn't matter, and how the other of those various groups does matter. Based on the factors involved in determining the answer. Once FizzBuzzExternal was created, it no longer had to change to get the new rules.
More rules
There is a type of rule that I'm not sure how it'll fit, and really depends on where it belongs in the order. This rule is "has digit".
If the value has the digit 3, then it should return "Fizz":
Where does this fit?
Is it equivalent of the multiple of 3? Is it highest priority? Is it only higher than the default response?
If it's the highest priority, for any value with a 3 digit we'd get "Fizz" back. This is an easy implementation even if it's fairly forced.
If it's equal to MultipleOf, then the order of the 3 and the 5 factor start to matter.... Bad example. Those will always get picked up by FizzBuzz.
OK, 3 and 5 and 7 order matter. Take 380 (Thanks Excel!). It's a multiple of 7 and 5, but not 3. What should it return?
We're shifting from factors to ... something else... that determines the order.
I'd expect the Inclusion of these new rules will help to further generalize the rule set, but I'm not going to do that; at lest now now.
The goal was to update the FizzBuzz object to allow rule extension without modification, and now we have.
TA-DA!