2
\$\begingroup\$

While implementing the Retry & Breaker patterns I decided that the Breaker does more then it should so I extracted two responsibilities into their own classes. Here they are.

I stripped out the Threshold to be only data:

public class Threshold
{
    public Threshold(int count, TimeSpan interval, TimeSpan timeout)
    {
        Count = count;
        Interval = interval;
        Timeout = timeout;
    }

    public int Count { get; }

    public TimeSpan Interval { get; }

    public TimeSpan Timeout { get; }

    public override string ToString()
    {
        return $"Count = {Count} Interval = {Interval} Timeout = {Timeout}";
    }
}

The second responsibility is the object counting the events and checking if the threshold is exceeded. I call it Fuse and implemented it this way:

public class Fuse
{
    public Fuse(Threshold threshold)
    {
        Threshold = threshold;
    }

    // todo: add null check
    public IClock Clock { get; set; } = new SystemClock();

    public Threshold Threshold { get; }

    public int Count { get; private set; }

    public DateTime? Point { get; private set; }

    public bool Blown
    {
        get
        {
            return
                Clock.GetUtcNow() - Point <= Threshold.Interval &&
                Count >= Threshold.Count;
        }
    }

    public bool TimedOut
    {
        get { return (Clock.GetUtcNow() - Point) > Threshold.Timeout; }
    }   

    public Fuse Increase(int value)
    {
        if (TimedOut) { Reset(); }
        Count += value;
        Point = Clock.GetUtcNow();
        return this;
    }

    public Fuse Increase()
    {
        Increase(1);
        return this;
    }

    public Fuse Reset()
    {
        Count = 0;
        Point = null;
        return this;
    }

    public override string ToString()
    {
        return $"Count = {Count} Point = \"{Point?.ToString(CultureInfo.InvariantCulture)}\" Blown = {Blown} TimedOut = {TimedOut}";
    }
}

To be able to better test it I created an abstraction for the DateTime and called it IClock:

public interface IClock
{
    DateTime GetNow();
    DateTime GetUtcNow();
}

public class SystemClock : IClock
{
    public DateTime GetNow() => DateTime.Now;
    public DateTime GetUtcNow() => DateTime.UtcNow;
}

public class TestClock : IClock
{
    public DateTime Now { get; set; }
    public DateTime UtcNow { get; set; }
    public DateTime GetNow() => Now;
    public DateTime GetUtcNow() => UtcNow;
}

Without the breaker the two new modules are now easier to test:

var threshold = new Threshold(count: 3, interval: TimeSpan.FromSeconds(5), timeout: TimeSpan.FromSeconds(10));
var fuse = new Fuse(threshold) { Clock = new TestClock { UtcNow = new DateTime(2016, 11, 12, 9, 0, 0) } };
fuse.Increase(2).ToString().Dump();
fuse.Increase().ToString().Dump();
(fuse.Clock as TestClock).UtcNow = new DateTime(2016, 11, 12, 9, 0, 0).AddSeconds(20);
fuse.ToString().Dump();
fuse.Increase().ToString().Dump();

Output:

Count = 2 Point = "11/12/2016 09:00:00" Blown = False TimedOut = False
Count = 3 Point = "11/12/2016 09:00:00" Blown = True TimedOut = False
Count = 3 Point = "11/12/2016 09:00:00" Blown = False TimedOut = True
Count = 1 Point = "11/12/2016 09:00:20" Blown = False TimedOut = False
\$\endgroup\$
1
\$\begingroup\$

Your interface Iclock exposes methods GetNow and GetUtcNow. Your TestClock class implements it, but because you also want to be able to modify the values you end up by implementing the properties Now and UtcNow.

In my opinion would be a better alternative that IClock would expose a read-only properties, and that your class would expose the public setter as well:

public interface IClock
{
    DateTime Now{ get; }
    DateTime UtcNow{ get; }
}

public class TestClock : IClock
{
    public DateTime Now { get; set; }
    public DateTime UtcNow { get; set; }
}

Your Increase method in Fuse class is hard to get grasp on.

So... I migth be wrong but you do not want to keep track of the current time when Increase method is called. What this means is that this line Point = Clock.GetUtcNow(); would become

Point = Point ?? Clock.GetUtcNow();

Furthermore I would consider to move this to some other method. It could be a wise choice to move it to the constructor, however if you want to have a better control about exactly which tim ite is related to you could have a start method.

public Fuse Start(){
    //if you want to you can check if Point is null and throw an exception if it's not
    Point = Clock.GetUtcNow();
}

It also seems that your Increase method sometimes decides to do his trick and call Reset method. If I were a consumer of any API I would never guess that a method called Increase would reset something.

In my opinion one simple thing you can do it have a boolean flag that would decide if Reset should be called "automatically".

Obviously this change would be accompanied by a slightly better named method. Unfortunately nothing better than ShortenFuse comes to mind.

public class Fuse
{
    public Fuse(Threshold threshold)
    {
        Threshold = threshold;
        AutoReset = true;
    }

    public bool AutoReset{ get; set; }

    public Fuse ShortenFuse(int value)
    {
        if (TimedOut && AutoReset) { Reset(); }
        Count += value;
        return this;
    }
    //...
}

One more thing. In your "tests" you do this:

(fuse.Clock as TestClock).UtcNow = new DateTime(2016, 11, 12, 9, 0, 0).AddSeconds(20);

Wouldn't it be much better to put the seconds on the constructor?

\$\endgroup\$
7
  • \$\begingroup\$ I agree about most points and I have I have to defend one solution. I've read a book about best practices lately where the creators of the .NET admit themselves that the DateTime.[Utc]Now should have been methods Get[Utc]Now because they return a new value each time and not just some data. You're right about the other points, especially about the unnecessary Point reset - I wasn't aware of this bug. \$\endgroup\$
    – t3chb0t
    Nov 12 '16 at 12:26
  • \$\begingroup\$ About the Increase method. I had a really hard time to come up with a name for it. Now after a while I have one more idea - perhaps Measure would be better. Other ideas I had were Count, Track, Register. \$\endgroup\$
    – t3chb0t
    Nov 12 '16 at 12:27
  • \$\begingroup\$ I didn't use the construtor for the TestClock because it was a copy/pase and somehow I didn't realize there might be another parameter :-o \$\endgroup\$
    – t3chb0t
    Nov 12 '16 at 12:28
  • \$\begingroup\$ Oh, and thanks to your review I have one more idea ;-D I make the Timeout optional (another constructor). If specified the Fuse will support AutoRest otherwise not. \$\endgroup\$
    – t3chb0t
    Nov 12 '16 at 12:33
  • 1
    \$\begingroup\$ @t3chb0t (About your first comment). Seems a reasonable reason. However if you have a reason to implement in that way you should prefer to implement the interface IClock explicitly so users of TestClock don't see both GetNow and Now \$\endgroup\$ Nov 12 '16 at 12:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.