How should I set a daily method as executed, from inside or from outside? [closed]

Question

I have the following code:

static void TimerElapsed(object sender, ElapsedEventArgs e)
{
    foreach (BIGService bigService in runningServices)
    {
        if (!bigService.ExecutedToday)
        {
            int executionResult = bigService.Execute();
            string serviceName = bigService.ToString();
            if (executionResult == 0)
            {
                EventLogger.WriteToEventLog("Success");
            }
            else
            {
                EventLogger.WriteToEventLog("Failure");
            }
        }
    }
}

where BIGService is an Abstract class, that has 5 descendants (5 individual service classes with this common parent). The enclosing timer's interval is 7200. So it elapses in every 2 hours.

All services must be run only once a day. Therefore I created a bool ExecutedToday property and void SetAsExecuted method.

My question is: where do you recommend I should call the SetAsExecuted method? Which way is more subservient? In the end of the Execute() method itself or in my copied code at the end of the if clause?

(Similar situations have happened to me several times, I just now have the time to ask other's opinion)

Answer 1

As your code stands now it looks to me as if the timer has some responsibilities it shouldent have.

My suggestion would be to either move all the timer information to your timer, including bigService.ExecutedToday and SetAsExecuted OR you move all that logic inside the service. I would go for the latter since that would be more in line with the "Tell dont ask" principle.

The code in the timer would look like this:

static void TimerElapsed(object sender, ElapsedEventArgs e)
{
    foreach (BIGService bigService in runningServices)
    {            
           bigService.Execute();           
    }
}

In your BigService method you contain the logging and wether or not that service can be run at the time it is requested.

One more thing: I would make an Enum to store the 0 value for executionResult, at a later date it might not be clear to you what 0 actually means

Answer 2

I prefer it if classes manage their own state as much as possible: that makes it difficult to use them incorrectly.

For example, in the code above you are able to 'forget' to call SetAsExecuted and can thus write a bug.

So if ExecutedToday is a property of Service then it's better if Service manages that property state.

Service
{
    DateTime recentlyExecuted;
    public bool ExecutedToday
    { get { return (DateTime.Now - recentlyExecuted) < new TimeSpan(1,0,0,0); } }
    public int Execute() {
        int executionResult = OnExecute(); // do the work
        if (executionResult == 0)
        {
            EventLogger.WriteToEventLog("Success");
        }
        else
        {
            EventLogger.WriteToEventLog("Failure");
        }
        recentlyExecuted = DateTime.Now; // or, only set if execution successful?
        return executionResult;
    }
    protected abstract int OnExecute(); // do subclass-specific work for Execute
}

If you want to ensure that Execute sets the property, then it should be a template method as coded above: otherwise (if Execute is abstract) then you're counting on every subclass to remember to set the property in its override of the Execute method.

Alternatively you can maintain the recentlyExecuted property outside the Service class:

static Map<BIGService, DateTime> recentlyExecuted = new Map<BIGService, DateTime>();

static void TimerElapsed(object sender, ElapsedEventArgs e)
{
    foreach (BIGService bigService in runningServices)
    {
        if (recentlyExecuted.ContainsKey(service) &&
            ((DateTime.Now - recentlyExecuted[service]) < new TimeSpan(1,0,0,0)))
            continue; // already executed today
        int executionResult = bigService.Execute();
        recentlyExecuted[service] = DateTime.Now; // or, only set if execution successful?
        string serviceName = bigService.ToString();
        if (executionResult == 0)
        {
            EventLogger.WriteToEventLog("Success");
        }
        else
        {
            EventLogger.WriteToEventLog("Failure");
        }
    }
}