Timer Tick Event only to execute at specific time

Question

I have a timer that is only supposed to tick between x and x on weekdays.

I have used the following implementation.

 void tmrMain_Tick(object sender, EventArgs e)
    {
        if (!(DateTime.Now.DayOfWeek == DayOfWeek.Saturday || DateTime.Now.DayOfWeek == DayOfWeek.Sunday))
        {
            if ((IsTimeOfDayBetween(DateTime.Now, new TimeSpan(8, 0, 0), new TimeSpan(18, 0, 0))))
            {
                if (pnlAssembly.Visible == true)
                {
                    cmdShed.PerformClick();

                }
                else if (pnlShed.Visible == true)
                {
                    cmdServices.PerformClick();
                }
                else if (pnlWeb.Visible == true)
                {
                    cmdAssembly.PerformClick();
                }
            }
        }
    }
    static bool IsTimeOfDayBetween(DateTime time,
                                  TimeSpan startTime, TimeSpan endTime)
    {
        if (endTime == startTime)
        {
            return true;
        }
        else if (endTime < startTime)
        {
            return time.TimeOfDay <= endTime ||
                time.TimeOfDay >= startTime;
        }
        else
        {
            return time.TimeOfDay >= startTime &&
                time.TimeOfDay <= endTime;
        }

    }

Is this the way to go?

Answer 1

Here is another alternative...

private void tmrMain_Tick(object sender, EventArgs e) 
{ 
    if (ShouldRunNow())
        PerformClick();
} 

private void PerformClick()
{
    if (pnlAssembly.Visible)
        cmdShed.PerformClick(); 

    else if (pnlShed.Visible) 
        cmdServices.PerformClick(); 

    else if (pnlWeb.Visible)
        cmdAssembly.PerformClick(); 
}

private bool ShouldRunNow()
{
    TimeSpan startTime = new TimeSpan(8, 0, 0); 
    TimeSpan endTime = new TimeSpan(18, 0, 0);
    DateTime now = DateTime.Now;

    // Only run Saturday and Sunday
    if (now.DayOfWeek != DayOfWeek.Saturday && now.DayOfWeek != DayOfWeek.Sunday)
        return false;

    // Only run between the specified times.
    if (endTime == startTime )
        return true; 

    if (endTime < startTime) 
        return now.TimeOfDay <= endTime || now.TimeOfDay >= startTime; 

    return now.TimeOfDay >= startTime && now.TimeOfDay <= endTime; 
}

Couple notes:
I think this is what ANeves meant by "would also consider extracting to a method the logic of whether or not to do the actions" but not sure so I decided to show it. By putting the entire decision tree in a method you have self documenting code (the "if (ShouldRunNow())" describes what you are trying to do without needing comments).

Eliminating "else" statements makes your code more readable (IMHO)

I personnally think using brackets when there is only one statement makes your code less readable as well. Of course, this is my own opinion and I even have people at my workplace that disagree with this. I will let the readers decide :)

Since the ShouldRunNow method is private then it doesn't hurt to not pass in the parameters. And putting them directly in the method will make them easier to find. This is not a big deal either way but just another alternative.

Update Changed code based on feedback in comments

Answer 2

I would use immediate returns to reduce the nesting of the first function.

I would extract the constants out.

I would also consider extracting to a method the logic of whether or not to do the actions.

private readonly TimeSpan StartTime = new TimeSpan(8, 0, 0);
private readonly TimeSpan EndTime = new TimeSpan(18, 0, 0);
private void tmrMain_Tick(object sender, EventArgs e) {
    if (DateTime.Now.DayOfWeek == DayOfWeek.Saturday
            || DateTime.Now.DayOfWeek == DayOfWeek.Sunday
            || !IsTimeOfDayBetween(DateTime.Now, StartTime, EndTime)
            ) {
        return;
    }
    if (pnlAssembly.Visible == true) {
        cmdShed.PerformClick();
    } else if (pnlShed.Visible == true) {
        cmdServices.PerformClick();
    } else if (pnlWeb.Visible == true) {
        cmdAssembly.PerformClick();
    }
}