4
\$\begingroup\$

I have a "manager" class that returns objects when requested. The if the required object already exists in the object collection it is returned, else a new one is created, added to the collection, and returned. I also need to periodically remove the objects from the collection when they're no longer required. This is done by the Purge() method.

Initially I had made the Purge method part of the Manager class and made it private, as the intention is to only ever call it via a timer, and only the Manager ever needs to know about it.

However due to problems with unit testing it, I moved the Purge method into its own class and made the method public (and added some other public methods for the Manager to use to control its behaviour). This also allowed me to unit test it nicely.

Now that I'm looking over it again however, I can't help feeling the "Purge" class just isn't justifying itself. It is effectively just one method, and really it's the Manager's business to control the lifetime of the objects. In fact I was thinking of renaming the class "ThrottlerLifetimeManager". Plus given that both classes modify the collection, that increases the synchronization headache.

So I want to know, from a Single Responsibility perspective, whether the Purge functionality should be in its own class or just be a public method on the Manager class. Bearing in mind I want to be able to unit test the code.

Manager class:

public class ThrottlerManager : IThrottlerManager
{       
    private readonly IThrottlerFactory factory;
    private readonly IThrottlerCollection throttlers;
    private readonly IThrottlerPurger purger;
    private volatile bool shouldStop = false;

    public ThrottlerManager(IThrottlerFactory factory, IThrottlerCollection throttlers, IThrottlerPurger purger)
    {
        if (factory == null) throw new ArgumentNullException("factory");
        if (throttlers == null) throw new ArgumentNullException("throttlers");
        if (purger == null) throw new ArgumentNullException("purger");

        this.factory = factory;
        this.throttlers = throttlers;
        this.purger = purger;
    }


    public void Start()
    {
        var t = new TimeSpan(0, 1, 0);
        purger.StartAsyncPurge(t, t);
    }

    public void Stop()
    {
        lock (throttlers)
        {
            shouldStop = true;
            purger.StopAsyncPurge();
            StopAllThrottlers();
        }
    }

    private void StopAllThrottlers()
    {
        var running = throttlers.Values.ToList();
        running.ForEach(t => t.Stop());
        running.ForEach(t => t.WaitForThrottlerToStop());
    }

    public void ThrottleProcess(IMonitoredProcess process)
    {
        lock (throttlers)
        {
            if (shouldStop) return;

            var throttler = GetThrottlerFor(process);
            throttler.ThrottleProcess(process);
        }
    }

    private IThrottler GetThrottlerFor(IMonitoredProcess process)
    {
        var config = process.Config;
        var key = GetKeyFor(config);

        IThrottler throttler;
        if (throttlers.TryGetValue(key, out throttler))
        {
            if (throttler.IsAlive) return throttler;
            else throttlers.Remove(key);
        }

        throttler = factory.CreateThrottlerFor(config);
        throttlers.Add(key, throttler);
        return throttler;
    }

    private string GetKeyFor(IMonitoredProcessConfig process)
    {
        return process.SuspendTime.ToString() + "/" + process.ResumeTime.ToString();
    }


}

Purge class:

public class ThrottlerPurger : IThrottlerPurger
{
    private readonly IThrottlerCollection throttlers;
    private readonly ILogger logger;
    private readonly Timer timer;

    public ThrottlerPurger(IThrottlerCollection throttlers, ILogger logger)
    {
        if (throttlers == null) throw new ArgumentNullException("throttlers");
        if (logger == null) throw new ArgumentNullException("logger");

        this.throttlers = throttlers;
        this.logger = logger;
        timer = new Timer(x => Purge());
    }

    public void StartAsyncPurge(int startWhenInMs, int recurWhenInMs)
    {
        timer.Change(startWhenInMs, recurWhenInMs);
    }

    public void StartAsyncPurge(TimeSpan startWhen, TimeSpan recurWhen)
    {
        timer.Change(startWhen, recurWhen);
    }

    public void StopAsyncPurge()
    {
        timer.Change(Timeout.Infinite, Timeout.Infinite);
    }

    public void Purge()
    {
        Dictionary<string, IThrottler> toRemove = null;

        lock (throttlers)
        {
            foreach (var throttler in throttlers)
            {
                if (throttler.Value.IsIdle || !throttler.Value.IsAlive)
                {
                    if (toRemove == null) toRemove = new Dictionary<string, IThrottler>();
                    toRemove.Add(throttler.Key, throttler.Value);
                }
            }

            if (toRemove == null) return;

            foreach (var throttler in toRemove)
            {
                throttlers.Remove(throttler.Key);
                logger.LogInfo("Purged {0} {1} throttler.", throttler.Value.IsAlive ? "idle" : "dead", throttler.Key);
                throttler.Value.Stop();
            }
        }
    }


}
\$\endgroup\$
  • \$\begingroup\$ Hi! Welcome to Code Review. The title should only state the purpose of the code. \$\endgroup\$ – TheCoffeeCup Jan 11 '16 at 21:52
  • \$\begingroup\$ @TheCoffeeCup Is that better? I'm used to SO... \$\endgroup\$ – 404 Jan 11 '16 at 21:54
  • \$\begingroup\$ Yes, it's much better. \$\endgroup\$ – TheCoffeeCup Jan 11 '16 at 21:54
  • \$\begingroup\$ It would be nice if you added the interface too. \$\endgroup\$ – Heslacher Jan 12 '16 at 9:14
2
\$\begingroup\$

ThrottlerPurger

The Purge() method is looking somehow strange. Why do you create the Dictionary<string, IThrottler> toRemove outside of the lock and why do you assign null to it ?

The following is doing the same but cleaner

public void Purge()
{

    lock (throttlers)
    {
        var toRemove = new Dictionary<string, IThrottler>();
        foreach (var throttler in throttlers)
        {
            if (throttler.Value.IsIdle || !throttler.Value.IsAlive)
            {
                toRemove.Add(throttler.Key, throttler.Value);
            }
        }

        foreach (var throttler in toRemove)
        {
            throttlers.Remove(throttler.Key);
            logger.LogInfo("Purged {0} {1} throttler.", throttler.Value.IsAlive ? "idle" : "dead", throttler.Key);
            throttler.Value.Stop();
        }
    }
}  

and if IThrottlerCollection is implementing IEnumerable<T> we can make it shine like so

public void Purge()
{

    lock (throttlers)
    {
        var toRemove = throttlers.Where(throttler => IsRemoveable(throttler))
                                 .ToDictionary(throttler => throttler.Key, throttler => throttler .Value);

        foreach (var throttler in toRemove)
        {
            throttlers.Remove(throttler.Key);
            logger.LogInfo("Purged {0} {1} throttler.", throttler.Value.IsAlive ? "idle" : "dead", throttler.Key);
            throttler.Value.Stop();
        }
    }
}
private bool IsRemoveable(IThrottler throttler)
{
    return throttler.Value.IsIdle || !throttler.Value.IsAlive;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks. I suppose toRemove could be declared anywhere (no special reason why I made it outside the lock). However I don't initialize because I don't want to create a new object unnecessarily. In most cases it won't need to purge anything, hence I only create the Dictionary if I find I need it. I do like the IsRemovable method though. \$\endgroup\$ – 404 Jan 12 '16 at 9:58

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.