C#→ Критика класса LiveInternet

Янв 12, 2011

После первого опыта написания класса и программы на C#, я попросил знатоков этого языка оценить свежеиспеченный код. Для этого был направлен вопрос на форум сайта RSDN. Спустя несколько минут, я получил полный и развернутый ответ с указанием на все недочеты и узкие места в моем коде. Давайте посмотрим, как можно улучшить написанный ранее класс.

Свойства

1. Пример из класса:

public Uri URL
{
  set
  {
    _url = value;
  }
}

Данный код отвечает за установку ссылки из внешнего кода т.е. свойство (вспоминаем аналогию с Delphi). Здесь мне посоветовали следующее, цитирую:

Write-only свойства — плохая практика. Лучше заменить на метод вида SetUrl()

Таким образом, если свойство используется только на приемку параметров, лучше преобразовать его в метод:

public void SetUrl(Uri url)
{
    _url = url;
}

События

1. Пример из класса:

public delegate void CategoryEventHandler(object sender, CategoryEventArgs e);
public event CategoryEventHandler CategoryReceived;

Здесь мы создаем делегат для события, а потом объявляем само событие. Мне указали, что это лишнее и можно использовать обобщенный делегат EventHandler<>:

public event EventHandler<CategoryEventArgs> CategoryReceived;

2. Пример из класса:

if (ThreadStarted != null)
{
  ThreadStarted(this, new ThreadEventArgs(catThread));
}

Этот код используется для генерации события из класса, для того чтобы внешний код мог его обработать. Мне рекомендовали заменить вызов события виртуальным protected методом, чтобы класс-наследник мог влиять на логику генерации события:

protected virtual void OnThreadStarted(object sender, Thread catThread)
{
  if (ThreadStarted != null)
  {
    ThreadStarted(this, new ThreadEventArgs(catThread));
  }
}

После чего в коде класса можно будет указать компактный вызов метода (события):

OnThreadStarted(this, Thread.CurrentThread);

А если пользователю потребуется, он сможет переопределить данное событие в своем классе, которое должно наследовать LiveInternet класс.

Деструкторы

1. Пример из класса:

HttpWebResponse webResponse = (HttpWebResponse)webRequest.GetResponse();
Stream streamResponse = webResponse.GetResponseStream();
StreamReader readerResponse = new StreamReader(streamResponse);
...
webResponse.Close();
streamResponse.Close();
readerResponse.Close();

В этом примере мы создаем и уничтожаем экземпляры классов, которые реализуют интерфейс IDisposable. Поэтому в данном случае можно применить второе назначение директивы using:

using (HttpWebResponse webResponse = (HttpWebResponse)webRequest.GetResponse())
{
  ...
  using (Stream streamResponse = webResponse.GetResponseStream())
  {
    ...
    using (StreamReader readerResponse = new StreamReader(streamResponse))
    {
      ...
    } // здесь уничтожается readerResponse
    ...
  } // здесь уничтожается streamResponse
  ...
} // здесь уничтожается webResponse

Синхронизация

1. Пример из класса:

lock (this)
{
  ...
}

Здесь мы блокируем объект this для выполнения кода внутри в потокобезопасном режиме. Так как согласно MSDN:

lock (this) может привести к проблеме, если к экземпляру допускается открытый доступ

Поэтому лучше объявить отдельный объект и блокировать его:

private object lockThread;
lock (lockThread)
{
  ...
}

Также мне рекомендовали вместо явного создания потоков использовать асинхронные методы класса WebRequest — BeginGetResponse/EndGetResponse.