Добрый день!

Написал класс и на основе неё программу.
Хотелось бы услышать жесткую критику кода, с указание на "ошибки" и недочеты в коде.

Код класса —
Исходный код —

P.S. Это мой первый опыт написания программы на C#.

Заранее, спасибо!

Код класса — здесь
Исходный код — здесь

Здравствуйте, TDLite, Вы писали:

TDL>Хотелось бы услышать жесткую критику кода, с указание на "ошибки" и недочеты в коде.

Что сразу бросилось в глаза:

public string url;

Public-переменные — это плохо. Особенно не readonly. Не только в C#. Используйте автосвойства для простых случаев.

public List<Category> listCategory { get; private set; }

Наименования свойств гайдлайны рекомендуют формировать в PacalCase.
List<> — не лучший вариант для публичного контракта, тем более, НЯП, в контексте задачи его менять не предполагается.
Я бы выставил наружу IEnumerable<Category>.

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

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

set
{
  if (value >= 0 && value < _listCategory.Count)
  {
    _currentCategory = value;
  }
}

Сеттер, результат выполнения которого для клиентского кода не очевиден. Если value не попадет в заданный диапазон, то вы его просто не присвоете. При этом клиентский код будет уверен, что оно-таки присвоилось. Нужно как-то информировать клиентский код (бросать исключение, возвращать false из метода SetCurrentCategory... ).

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

"Лишний" тип делегата. Используйте обобщенный EventHandler<> (см. гайдлайны по реализации событий).

_stopExecution = false

Лишнее телодвижение. Переменные типа Boolean инициализируются в false по умолчанию.

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

Для генерации события нужен отдельный метод. С учетом того, что ваш класс не sealed, метод нужно делать protected virtual, чтобы класс-наследник мог влиять на логику генерации события. Ссылку на event надо бы сохранять в отдельной переменной. Опять же, см. гайдлайны по реализации событий.

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

WebResponse и его наследники, Stream и StreamReader реализуют IDisposable. Используйте using.

lock (this)

Why is lock(this) {…} bad?

catch (Exception e)

Why catch(Exception)/empty catch is bad.

catThread.Abort()
Thread.CurrentThread.Abort()

Не лучший способ завершать потоки. Поток нужно уведомлять, что ему следует прекратить работу (через флаги/объекты синхронизации...).
Для того, чтобы завершить поток, достаточно выйти через return из thread function. Это не только в .NET.
CurrentThread.Abort — это готично.

Вместо явного порождения кучи потоков, думаю, можно было бы обойтись асинхронным получением WebResponse через BeginGetResponse/EndGetResponse. См. Asynchronous Programming Design Patterns.

Наверняка, что-то пропустил, более сведущие дополнят.

З.З.Ы. Если позволяет версия студии, запустите Code Analysis на вашем проекте. Увидите много интересного.
З.Ы. Заглянул в ваш уютный бложик. Конкретно, глянул вот этот пост. Не в обиду, начните читать тематические книжки. С Рихтера (clr via c#), например.

HowardLovekraft, спасибо.
Узнал много нового, буду изучать.
Я так понимаю если есть асинхронные методы, они приоритетней явного создания потоков?

HL>Public-переменные — это плохо. Особенно не readonly. Не только в C#. Используйте автосвойства для простых случаев.

И чем же public get, set автосвойство лучше поля (readonly не рассматриваем — тут свойства вообще не в тему)?

Здравствуйте, -VaS-, Вы писали:

VS>И чем же public get, set автосвойство лучше поля (readonly не рассматриваем — тут свойства вообще не в тему)?
См., например, здесь:

    * Reflection works differently on variables vs. properties, so if you rely on reflection, it's easier to use all properties.
    * You can't databind against a variable.
    * Changing a variable to a property is a breaking change.

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

И это если игнорировать тот факт, что с т.з. ООП объект должен инкапсулировать свое состояние.

HL>От себя могу добавить, что ситуация, когда автосвойство переделывается в свойство, где как минимум сеттер содержит логику, более чем реальна.
HL>Если вы раньше использовали переменную, а не (авто)свойство, объяснить своим коллегам, почему им нужно выпустить по новой версии их проектов будет сложно.
HL>А если вы пишете библиотечный класс, который будет использовать кто-то за пределами вашей компании...

Не совсем понял, почему поле -> свойство есть breaking change.

HL>И это если игнорировать тот факт, что с т.з. ООП объект должен инкапсулировать свое состояние.

Свойства не являются механизмом инкапсуляции. Так же, как и методы, возвращающие состояние объекта.

Здравствуйте, TDLite, Вы писали:

TDL>Я так понимаю если есть асинхронные методы, они приоритетней явного создания потоков?
Скорее, если есть возможность использовать ThreadPool, лучше использовать его, чем явно создавать потоки.

Здравствуйте, -VaS-, Вы писали:

VS>Не совсем понял, почему поле -> свойство есть breaking change.
Ну там же дальше по ссылкам в дискуссии разжевано.

VS>Свойства не являются механизмом инкапсуляции. Так же, как и методы, возвращающие состояние объекта.
В чем принципиальное отличие свойства от пары методов Get/Set, кроме синтаксического сахара?

Здравствуйте, HowardLovekraft, Вы писали:

HL>

HL>public List<Category> listCategory { get; private set; }
HL>

HL>Наименования свойств гайдлайны рекомендуют формировать в PacalCase.
HL>List<> — не лучший вариант для публичного контракта, тем более, НЯП, в контексте задачи его менять не предполагается.
HL>Я бы выставил наружу IEnumerable<Category>.

А что делать в случае если клинетский код нуждается, например, в доступе по индексу и/или получении количества элементов в подобной коллекции? Имеет ли смысл в данном случае выставить наружу IList без сеттера и возвращать список только для чтения через List<T>.AsReadonly? Можно, конечно использовать LINQ для доступа по индексу и получения количества элементов в IEnumerable, но выглядит это как то не комильфо.

Здравствуйте, HowardLovekraft, Вы писали:

HL>

HL>catch (Exception e)
HL>

HL>Why catch(Exception)/empty catch is bad.

Соглашусь со всем, кроме этого.
В большинстве случаев ваше замечание было бы верным, но при написании функции, которая будет выполняться в потоке, очень важно тело функции обернуть в try-catch, иначе неожиданное исключение приведет к завершению приложения. Вы конечно можете возразить, что мол тогда нужно указать все конкретные типы исключений, которые могут быть, но на практике мы часто не можем на 100% быть уверены, что знаем все типы исключений, которые могут быть выброшены. Так что в случае с функцией потока, я бы как раз использовать catch(Exception ex).

Здравствуйте, HowardLovekraft, Вы писали:

HL>З.Ы. Заглянул в ваш уютный бложик. Конкретно, глянул вот этот пост. Не в обиду, начните читать тематические книжки. С Рихтера (clr via c#), например.

Рихтер, это конечно круто, но для молодого неокрепшего мозга может быть тяжело. Я просто видел, как тяжело читался Рихтер парнями, которые до этого пару лет кое как программировали на Delphi, а потом решили перейти на C# и начать с Рихтера.
В качестве первой книги по C# я бы порекомендовал Албахари (C# 4.0 in a Nutshell), по крайней мере если нет проблем с английским. Если есть проблемы с английском, то... желательно подучить англиский Иначе так и придется в 2011 году читать переведенного на русский Рихтера по C# & .NET 2.0 (2005).

Здравствуйте, cybosser, Вы писали:

C>А что делать в случае если клинетский код нуждается, например, в доступе по индексу и/или получении количества элементов в подобной коллекции? Имеет ли смысл в данном случае выставить наружу IList без сеттера и возвращать список только для чтения через List<T>.AsReadonly? Можно, конечно использовать LINQ для доступа по индексу и получения количества элементов в IEnumerable, но выглядит это как то не комильфо.

Этот момент в принципе заслуживает отдельной темы. Нужно начать с того, что решить а действительно ли нужно защищать коллекцию от изменения. Если это ваш внутренний проект, то можно вообще не заморачиваться. Если же все-таки защищить от изменения нужно, то IList<T>.AsReadOnly() это неплохой вариант (чтобы каждый раз не создавать новый объект-обертку при вызове свойства, можно его закешировать). Если выставлять наружу IEnumerable<T>, то, во-первых, коллекцию все равно можно будет изменить ( например так ((IList<T>)enumerable).Add(...) ), во-вторых будут проблемы с индексацией и получением количества элементов. Кстати, упомяну в тему гайдлайн, что если у IEnumerable<T> вам не хватает только получения количества элементов, то не стоит только из-за этого возвращать например IList<T>, вместо этого можно перед итерацией попробовать привести коллекцию (IEnumerable<T>) к ICollection<T> и если приведение выполнится, то воспользоваться свойством Count. То же самое кстати делает и extension method IEnumerable<T>.Count(), он сначала пытается привести к ICollection<T> и только потом, если не получилось, итерирует коллекцию чтобы посчитать количество элементов.

Здравствуйте, MozgC, Вы писали:

MC>Этот момент в принципе заслуживает отдельной темы. Нужно начать с того, что решить а действительно ли нужно защищать коллекцию от изменения. Если это ваш внутренний проект, то можно вообще не заморачиваться.
Насколько я понял, компонент предполагает некий реюз, возможно в разных проектах

MC>Если же все-таки защищить от изменения нужно, то IList<T>.AsReadOnly() это неплохой вариант (чтобы каждый раз не создавать новый объект-обертку при вызове свойства, можно его закешировать). Если выставлять наружу IEnumerable<T>, то, во-первых, коллекцию все равно можно будет изменить ( например так ((IList<T>)enumerable).Add(...) ), во-вторых будут проблемы с индексацией и получением количества элементов.
Для IEnumerable тоже можно сделать и закэшировать обертку как и List<T>.AsReadOnly() (не IList<T>, кстати).
categories.Where(_ => true);
или
categories.Select(_ => _);
криво, но раз нет спецметода, то пойдет...

MC>Кстати, упомяну в тему гайдлайн, что если у IEnumerable<T> вам не хватает только получения количества элементов, то не стоит только из-за этого возвращать например IList<T>, вместо этого можно перед итерацией попробовать привести коллекцию (IEnumerable<T>) к ICollection<T> и если приведение выполнится, то воспользоваться свойством Count. То же самое кстати делает и extension method IEnumerable<T>.Count(), он сначала пытается привести к ICollection<T> и только потом, если не получилось, итерирует коллекцию чтобы посчитать количество элементов.

Да, все так, пока Enumerable.Count() обращается к чему-то, реализовывающему ICollection<T>. Однако, как только сделали обертку через Where или Select (см выше), то Enumerable.Count() и ElementAt() начинают работать с линейной сложностью.

Потому, если хочется защититься от изменений и обеспечить константную сложность при обращении по индексу/взятию числа элементов, то List<T>.AsReadOnly() — все-таки лучшее, что есть из коробки для этого.

Здравствуйте, MozgC, Вы писали:

MC>В большинстве случаев ваше замечание было бы верным, но при написании функции, которая будет выполняться в потоке, очень важно тело функции обернуть в try-catch, иначе неожиданное исключение приведет к завершению приложения.

В этом случае лучше использовать BeginInvoke, в AsyncCallback использовать syncContext оригинального потока, и вызывать EndInvoke в оригинальном потоке. Или, что то же самое, но несколько проще, использовать async.

catch-ить надо _только_ ожидаемые исключения, которые вы знаете как обработать. Всё остальное — ересь и прямая дорога к "чожонопадает-то?".

Здравствуйте, Sinix, Вы писали:

S>catch-ить надо _только_ ожидаемые исключения, которые вы знаете как обработать. Всё остальное — ересь и прямая дорога к "чожонопадает-то?".
Прямая дорога к "чожонопадает-то", это, как я уже написал, пропустить непредусмотренное исключение, из-за которого все приложение навернется. Такое допустимо в обычных (не потоковых) методах, потому что обработчики верхнего уровня спасут приложение от закрытия, а вот от исключения в потоке они не спасут.
Я не знаю как у вас, но мои пользователи не будут рады, если всё приложение закроется, когда какой-нибудь компонент используемый в потоке вдруг выбросит недокументируемое исключение, или даже просто исключение, которое я по невнимательности пропустил.

Здравствуйте, MozgC, Вы писали:

MC>при написании функции, которая будет выполняться в потоке, очень важно тело функции обернуть в try-catch,
MC>иначе неожиданное исключение приведет к завершению приложения. Вы конечно можете возразить, что мол тогда нужно указать все конкретные MC>типы исключений, которые могут быть, но на практике мы часто не можем на 100% быть уверены, что знаем все типы исключений, которые могут MC>быть выброшены. Так что в случае с функцией потока, я бы как раз использовать catch(Exception ex).
Насчет "обернуть в try-catch" — разве что вот так:

try
{
  // ...
}
catch (Exception e)
{
  // write to log
  ...
  // re-throw  
  throw;
}

Нежиданное исключение как раз должно приводить к завершению приложения. Именно потому, что оно неожиданное.
Логику восстановления после него предусмотреть в общем случае нельзя.
Следовательно, гарантировать, что с данными приложения все ОК, тоже нельзя (если приложение не hello-world).
Лучший выход в этом случае, IMHO, — попытаться записать исключение в лог и завершить работу.
Т.е. в конце обязательно должен присутствовать throw.

Если очень хочется автоматического перезапуска, то это достаточно легко решается путем настроек/watchdog'ов.

Здравствуйте, cybosser, Вы писали:

C>А что делать в случае если клинетский код нуждается, например, в доступе по индексу и/или получении количества элементов в подобной коллекции?
Про List<T>.AsReadonly ответил

Автор: samius
Дата: 11.01.11

samius.
Кмк, контракт должен быть настолько "бедным", насколько это возможно.
А "возможно" уже зависит от вероятных сценариев использования.

Здравствуйте, HowardLovekraft, Вы писали:

HL>Нежиданное исключение как раз должно приводить к завершению приложения. Именно потому, что оно неожиданное.

Ну да, у представляю:

Директор:
— Паша, что за фигня, почему я 20 минут готовил документы, потом нажал на Help и приложение закрылось?
— Все нормально, Саш, все по гайдлайнам, там один компонентик выкинул недокументированное (неожиданное) исключение при открытии хелпа, а мы такие исключения ловить не должны — закрываемся, все по правилам!
— ... (Директор посылает меня с такими правилами кое куда)

Здравствуйте, MozgC, Вы писали:

MC>Прямая дорога к "чожонопадает-то", это, как я уже написал, пропустить непредусмотренное исключение, из-за которого все приложение навернется. Такое допустимо в обычных (не потоковых) методах, потому что обработчики верхнего уровня спасут приложение от закрытия, а вот от исключения в потоке они не спасут.
Я уже написал, как такие проблемы решаются. Отлавливается обработчиком message loop.

MC>Я не знаю как у вас, но мои пользователи не будут рады, если всё приложение закроется, когда какой-нибудь компонент используемый в потоке вдруг выбросит недокументируемое исключение, или даже просто исключение, которое я по невнимательности пропустил.

Для таких операций код надо специально писать — если операция не удалась, откатывать _все_ изменения в finally, не отлавливая исключение. Затем, в обработчике завершения операции (в основном потоке), сообщать пользователю (и в лог) об ошибке операции. Вот тут

catch(Exception ex)
{
  if (!HandleException(ex)) throw;
}

вполне допустим. Только это очень частный случай.