2
\$\begingroup\$

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)

\$\endgroup\$

closed as off-topic by RubberDuck, Quill, Mast, Nic Hartley, IEatBagels Dec 20 '15 at 16:27

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Why not just set up a Scheduled Task on the machine this needs to run under? \$\endgroup\$ – Max Jan 17 '14 at 11:52
  • \$\begingroup\$ I don't really want to use that. I want my program to manage itself. \$\endgroup\$ – Mitulát báti Jan 20 '14 at 9:59
4
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Or bool instead of an Enum, if you want a boolean "succeeded-or-failed" result. \$\endgroup\$ – ChrisW Jan 17 '14 at 12:42
  • \$\begingroup\$ @CrisW Good point. If its a succeed/fail scenario there is really no need for an enum and a bool would give the desired clarity. \$\endgroup\$ – Helge Heldre Jan 17 '14 at 19:47
  • \$\begingroup\$ It's not an easy 1 bit scenario, there are more branches as results. Actually that is Enum, just here in this code is 0 (just for the sake of simplicity). Thanks a lot anyway! \$\endgroup\$ – Mitulát báti Jan 20 '14 at 10:04
2
\$\begingroup\$

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");
        }
    }
}
\$\endgroup\$

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