Правильное использование класса Timer - PullRequest
Новая
       17

Правильное использование класса Timer

3 голосов
/ 19 июля 2011

Я хотел бы знать, если код, который я написал, написан правильно, он работает, но я уже делал плохие проекты, поэтому мне нужно знать, правильно ли я думал.

Код - это использование System.Timers.Timer для повторения действий каждые X часов.Я прочитал некоторые темы на эту тему в stackoverflow, а затем попытался написать свой собственный класс.Вот что я написал: 

namespace MyTool
{
public  class UpdaterTimer : Timer
{
    private static UpdaterTimer _timer;
    protected UpdaterTimer()
    { }

    public static UpdaterTimer GetTimer()
    {
        if (_timer == null)
            _timer = new UpdaterTimer();

        SetTimer(_timer);
        return _timer;
    }

    private static void SetTimer(UpdaterTimer _timer)
    {
        _timer.AutoReset = true;
        _timer.Interval = Utils.TimeBetweenChecksInMiliseconds;
        _timer.Elapsed += new ElapsedEventHandler(_timer_Elapsed);
        _timer.Start();
        DoStuff();
    }

    static void _timer_Elapsed(object sender, ElapsedEventArgs e)
    {
        DoStuff();
    }

    private static void DoStuff()
    {
       //does stuff on each elapsed event occurrence
    }
}
}

Краткое описание:

  • попытался использовать шаблон синглтона, поскольку мне нужен только один таймер для работы
  • Я неуверен, что вызов метода DoStuff () внутри метода SetTimer () кажется избыточным.Но логика заключается в том, что при запуске приложения DoStuff () должен запускаться, а затем запускаться снова при каждом событии Timer.Elapsed.

Мои вопросы:

  1. Вы бы написали это поведение по-другому, учитывая спецификацию?
  2. Можно ли использовать в этом случае синглтон или нет смысла?

Ответы [ 3 ]

4 голосов
/ 19 июля 2011

Рискну предположить, что создание подкласса Timer очень мало полезно.Вы превратили около 7 строк кода в целую кучу раздувания.Учитывая, что ваш DoStuff метод является закрытым, его нельзя использовать повторно.Поэтому рассмотрим следующие несколько строк: 

Action doStuff=()=>{
    //does stuff on each elapsed event occurance
};
_timer=new System.Timers.Timer(){
    AutoReset = true,
    Interval = Utils.TimeBetweenChecksInMiliseconds
};
_timer.Elapsed += (s,e)=>doStuff();
_timer.Start();
doStuff();

, где _timer - это свойство содержащего класса.Намерение здесь ясно.Я не думаю, что он заслуживает отдельного класса.

Edit: 

(s,e)=>doStuff()

- это лямбда-выражение, которое определяет делегата, который принимает два параметра s и e (отправитель и событие).Поскольку это добавлено к событию _timer.Elapsed (типа ElapsedEventHandler), компилятор может вывести типы s и e из типа события ElapsedEventHandler.Единственное действие, которое выполняет наш делегат, это doStuff().

В произвольной форме это может быть расширено до: 

_timer.Elapsed += delegate(object sender, ElapsedEventArgs e){doStuff();};

Лямбда позволяет нам писать выше значительно более кратко.

2 голосов
/ 19 июля 2011

Я не знаю, чего вы пытаетесь достичь, но вот несколько моментов о вашем текущем дизайне:

  1. Ваш GetTimer сломан в режиме многопоточности:

    if (_timer == null) _timer = new UpdaterTimer();

    Предположим, у вас есть два потока, каждый из которых вызывает GetTimer одновременно, первый проверяет _timer и находит его нулевым, поэтому он продолжает.но в это время и до того, как он достигнет _timer = new UpdateTimer(), переключение контекста потока переключается на другой поток и приостанавливает выполнение текущего потока.поэтому другой поток проверяет _timer и находит его не равным NULL, поэтому он продолжает работу и создает новый таймер, теперь переключатель контекста переназначил первый поток и продолжил его выполнение, создав новый таймер и обновив старый.чтобы исправить использование одноэлементного шаблона, используйте вместо этого статический конструктор static UpdaterTimer() { _timer = new UpdaterTimer();}

  2. Разработчик может вызывать GetTimer() столько, сколько он хочет, поэтому он будет вызывать _timer.Elapsed += new ElapsedEventHandler(_timer_Elapsed);, снова регистрируя другое Elapsedобработчик.Также обратите внимание, что всякий раз, когда вы вызываете GetTimer, таймер запускается «_timer.Start()», даже если он был остановлен.

  3. Не возвращайте базовый таймер, вместо этого предоставьте открытые методыStart(), Stop(), UpdateInterval(int interval).

  4. В SetTimer() вы хотите немедленно вызвать DoStuff, но затем он будет заблокирован при ожидании метода SetTimerДля завершения DoStuff() лучше запустить этот метод в новом потоке ThreadPool.QueueUserWorkItem(new WaitCallback((_) => DoStuff())); или использовать System.Threading.Timer вместо System.Timers.Timer и установить его так, чтобы метод сразу вызывался.

2 голосов
/ 19 июля 2011

Я думаю, что вы должны использовать здесь композицию вместо наследования.(т.е. не наследуйте от Timer, просто используйте Timer как личное поле)
Также, если код вне вашего класса не должен изменять свойства таймера, вы не должны показывать его другим классам.Если вы хотите дать возможность внешнему коду подписаться на событие таймера Elapsed, вы можете добавить событие для него и вызвать его в обработчике DoStuff, если нет - просто скрыть его.

пример кода в соответствии с запросом OP в комментариях:
ПРИМЕЧАНИЕ: он не соответствует спецификации (это совершенно другой вариант использования, для вашего случая использования вам не нужен отдельный класс послевсе (см. другой ответ)), но показывает, как вы можете использовать композицию и скрыть детали реализации.(Также в примере есть некоторые проблемы с безопасностью потоков, но это не главное.) 

public static class TimerHelper
{
    private static Timer _timer;

    static TimerHelper()
    {
        _timer = new Timer(){AutoReset=true, Interval=1000};
        _timer.Start();
    }

    public static event ElapsedEventHandler Elapsed
    {
        add { _timer.Elapsed += value; } 
        remove { _timer.Elapsed -= value; }
    }
}

И использовать: 

// somewhere else in code
TimerHelper.Elapsed += new ElapsedEventHandler(TimerHelper_Elapsed);
...