Добрый день!

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

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

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;
}

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

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

S>Я уже написал, как такие проблемы решаются. Отлавливается обработчиком message loop.

Я же написал, что в рабочем потоке (вызыванном через Thread.Start()) ничего не отловится и приложение тупо закроется.

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

Не хотелось бы впадать во флейм, но вставлю пару копеек.

HL>Нежиданное исключение как раз должно приводить к завершению приложения. Именно потому, что оно неожиданное.
HL>Логику восстановления после него предусмотреть в общем случае нельзя.
А логика восстановления в общем случае может быть не нужна (в частном нужна), если код потока не производит деструктивных изменений данных.
Возьмем тот же пример ТС-а (особо не вчитывался). Но от того что мы напоремся на какие-то неожиданные исключения при серфинге в инете или разборе HTML, ничего страшного не случится. Ну не получим результат, об этом надо намекнуть пользователю (если приложение интерактивное) и двигать дальше.

Отдельные случаи — это критичные исключения, OOM и иже с ними, после которых восстановиться нельзя или очень сложно. Вот их можно предусматреть в отдельных катчах, после чего catch(Exception ex) будет довольно невинным.

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

MC>Я же написал, что в рабочем потоке (вызыванном через Thread.Start()) ничего не отловится и приложение тупо закроется.
А не надо рабочий поток вызывать как Thread.Start Или тогда уж будьте любезны переизобретать велосипед с EndInvoke.

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

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

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

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

Я вот не помню, как этот паттерн поведет себя при OutOfMemoryException, например? Делегирование исключения UI потоку может просто не случиться, на сколько я понимаю.

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

S>А не надо рабочий поток вызывать как Thread.Start Или тогда уж будьте любезны переизобретать велосипед с EndInvoke.

Ну это уже вопросы к топикстартеру, у него Thread.Start(). А ему посоветовали убрать catch(Exception ex). Я считаю данный совет неподходящим конкретно в данной ситуации ТС (с Thread.Start()).

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

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

S>>А не надо рабочий поток вызывать как Thread.Start;) Или тогда уж будьте любезны переизобретать велосипед с EndInvoke.

MC>Ну это уже вопросы к топикстартеру, у него Thread.Start(). А ему посоветовали убрать catch(Exception ex). Я считаю данный совет неподходящим конкретно в данной ситуации ТС (с Thread.Start()).

Я все понял. Не нужно холиваров)
Считаю, что катчить нужно все исключения, хотя бы для того, чтобы все это красиво выдать с просьбой записать нужную информацию на листок и закрыть приложение =).

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

S>Я вот не помню, как этот паттерн поведет себя при OutOfMemoryException, например? Делегирование исключения UI потоку может просто не случиться, на сколько я понимаю.

И не должно. Поздно пить боржоми

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

S>Не хотелось бы впадать во флейм
Это не реально. Сам не хотел писать про catch(Exception), но не сдержался. Exception-срач вечен.

Когда приводят такие

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

примеры, у меня обычно возникает вопрос "а почему у вас есть возможность редактировать 20 минут сферические документы в вакууме, но нет, например, автосохранения"? А если гнусные электрики будут рядом ремонтировать проводку, случайно выключат свет, а бесперебойник забрали в ремонт? Или один из драйверов уронит систему в BSOD?

S>Но от того что мы напоремся на какие-то неожиданные исключения при серфинге в инете или разборе HTML, ничего страшного не случится
Возможно. Смотря что от этого зависит.
Да, наверное, есть приложения, где можно ловить все подряд. Но, IMHO, это исключение.
По своей воле точно не стал бы такое делать в сервисах. Проще перезапуститься.

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

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

S>>Я вот не помню, как этот паттерн поведет себя при OutOfMemoryException, например? Делегирование исключения UI потоку может просто не случиться, на сколько я понимаю.

S>И не должно. Поздно пить боржоми

Тогда получается, что EndInvoke ничего толком и не делает, кроме как передачи исключения вызывающему коду...

Еще он имеет кое-какой недостаток — использует лишь потоки пула. А для долгих работ в фоне вроде не рекомендуется использовать потоки пула (не помню где читал).
Значит что велосипедостроения на тему EndInvoke вероятны. И там как раз пропишется catch(Exception)...

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

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

S>>А не надо рабочий поток вызывать как Thread.Start Или тогда уж будьте любезны переизобретать велосипед с EndInvoke.

MC>Ну это уже вопросы к топикстартеру, у него Thread.Start(). А ему посоветовали убрать catch(Exception ex). Я считаю данный совет неподходящим конкретно в данной ситуации ТС (с Thread.Start()).

Очень подходящий совет. Выполнение его подтолкнет к использованию более правильных средств (типа TPL).

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

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

S>>Не хотелось бы впадать во флейм
HL>Это не реально. Сам не хотел писать про catch(Exception), но не сдержался. Exception-срач вечен.


HL>Когда приводят такие

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

примеры, у меня обычно возникает вопрос "а почему у вас есть возможность редактировать 20 минут сферические документы в вакууме, но нет, например, автосохранения"? А если гнусные электрики будут рядом ремонтировать проводку, случайно выключат свет, а бесперебойник забрали в ремонт? Или один из драйверов уронит систему в BSOD?
Да, автосохранение — это безусловно выход. Но не дешевый. Catch(Exception) по отношению к автосохранению условно — бесплатен (при наличии как минимум логирования).

S>>Но от того что мы напоремся на какие-то неожиданные исключения при серфинге в инете или разборе HTML, ничего страшного не случится
HL>Возможно. Смотря что от этого зависит.
HL>Да, наверное, есть приложения, где можно ловить все подряд. Но, IMHO, это исключение.
Проблема несколько в другом. Есть приложения/библиотеки/сервисы (даже из числа довольно распространенных), из которых вылетает все подряд, и они регулярно обновляются, так что предусмотреть все исключения и обработать их в режиме "ожидаемых" нереально.

HL>По своей воле точно не стал бы такое делать в сервисах. Проще перезапуститься.
Когда ситуация с исключениями под контролем, такое и не нужно. Сам бы не стал такое писать. Но беда компонентного подхода именно в том, что наверняка не знаешь, что когда и откуда вылетит.
Т.е. я согласен, что catch(Exception) быть не должно в идеале. И изначально я catch(Exception) не использую. Вставляю его как затычку при использовании непредсказуемого 3rd party кода, например, когда он наверняка не может повлиять на дальнейшую работоспособность приложения. И такое случается чаще, чем хотелось бы.

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

TDL>Добрый день!

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

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

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

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

Если уж вот так выставляешь код на всеобщее обозрение, то хотя бы XML-комменты к публичным членам напиши.

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

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

Объясняю. Берете программулину FxCop, запускаете и натравливаете ее на вашу сборку. Она вам выдаст основные недочеты, связанные именно с языком и платформой. А остальное -- это уже общие вопросы программирования, их нужно годами изучать, да и то не каждый способности имеет.

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

S>Еще он имеет кое-какой недостаток — использует лишь потоки пула. А для долгих работ в фоне вроде не рекомендуется использовать потоки пула (не помню где читал).

Долгие работы — это часы и дни. Разумеется, если вы не забиваете весь пул фоновыми задачами — тогда любая работа будет долгой

S>Значит что велосипедостроения на тему EndInvoke вероятны. И там как раз пропишется catch(Exception)...
Да. Но это инфраструктурный код, и он пишется (в идеале) 1 раз за всю жизнь проекта. FDG в первую очередь нацелен на обычный код.

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

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

S>>Еще он имеет кое-какой недостаток — использует лишь потоки пула. А для долгих работ в фоне вроде не рекомендуется использовать потоки пула (не помню где читал).

S>Долгие работы — это часы и дни. Разумеется, если вы не забиваете весь пул фоновыми задачами — тогда любая работа будет долгой

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

S>>Значит что велосипедостроения на тему EndInvoke вероятны. И там как раз пропишется catch(Exception)...
S>Да. Но это инфраструктурный код, и он пишется (в идеале) 1 раз за всю жизнь проекта. FDG в первую очередь нацелен на обычный код.
Да, и код ТС инфраструктурным нельзя назвать.

Но вот засада — каждый раз, когда сталкиваюсь с нарушениями FDG или принципов OOP в инфраструктурном коде (в своем или в FCL), кажется что что-то в консерватории не так. То ли не так с категоричностью принципов, то ли инфраструктурный код имеет индульгенцию

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

S>Ключевым здесь является именно угроза забивания пула, а забить его можно и секундными задачами при некоторой сноровке и требованиями к латентности приложения.
Решается использованием инструментов по назначению. Для нанозадач есть TPL.

S>Да, и код ТС инфраструктурным нельзя назвать.
Да, всё смешано Лучше бы разнести часть логики в хелперы, избавиться от логики на Thread.Abort и от состояния, разделяемого между потоками. Вы учтите, что у топикстартера — первый код на шарпе, и само по себе решение вполне рабочее Только поддерживать такой код неудобно.

S>То ли не так с категоричностью принципов, то ли инфраструктурный код имеет индульгенцию
FDG — это рекомендации. Их надо понимать, а не слепо использовать. Назовёте конкретные случаи — отвечу, сферокод тут обсуждать бессмысленно.

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

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

S>>Ключевым здесь является именно угроза забивания пула, а забить его можно и секундными задачами при некоторой сноровке и требованиями к латентности приложения.
S>Решается использованием инструментов по назначению. Для нанозадач есть TPL.
Мысль о недостатках BeginInvoke я ввел лишь к тому что catch(Exception) писать иногда приходится. Да, TPL есть, но все случаи необходимости catch(Exception) он наверняка не перекрывает. Примеры высасывать не хочу, потому предлагаю на этом остановиться.

S>>Да, и код ТС инфраструктурным нельзя назвать.
S>Да, всё смешано Лучше бы разнести часть логики в хелперы, избавиться от логики на Thread.Abort и от состояния, разделяемого между потоками. Вы учтите, что у топикстартера — первый код на шарпе, и само по себе решение вполне рабочее Только поддерживать такой код неудобно.
Я сюда влез не по просьбе ТС о критике
Но так и быть. Да, именно что всё смешано. Я полагаю что в коде на Delphi у ТС тоже смешано. Это уже вопрос не языка/платформы, а стиля, чувства прекрасного и т.п.

S>>То ли не так с категоричностью принципов, то ли инфраструктурный код имеет индульгенцию
S>FDG — это рекомендации. Их надо понимать, а не слепо использовать.
Согласен
А влез я сюда докопаться до категоричности утверждения

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

S>Назовёте конкретные случаи — отвечу, сферокод тут обсуждать бессмысленно.
я пас. Не хотел обсуждать ни конкретные, ни сферические случаи. Только то что ересью и дорогами к "чожонопадает-то?" пользоваться приходится.

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

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

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

И правильно делает. При разговоре (тем более) с начальством необходимо быть точным и честным. Не нужно на "гайдлайны" перекладывать свои ошибки или недостатки процесса, который не протестировал обычное "нажал на Help". В сложных же случаях как раз "правильные практики" позволяют выявить и определить ошибку.

Берём пример со студии: чуть что вылетает. Но есть возможность востановить редактировавшиеся файлы.

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

S>А влез я сюда докопаться до категоричности утверждения

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

rsdn такой rsdn... Пока не напишешь дисклаймер на 5 страниц "не рассматриваем очень частные случаи, включаем свои мозги" — не отстанут

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

Понятно. Для библиотек согласен. Но делать всегда свойства вместо полей — паранойя. Смешно видеть у того же Фаулера в книжке по DSL вот такое:

class Activity...
  public string Type { get; set; }
  public int Amount { get; set; }
  public int Revenue { get; set; }
  public string Location { get; set; }

Это чисто data-класс. Никакого поведения там никогда не будет.

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

Ни в чем, о том и речь. Я лишь хотел сказать, что обернув приватное поле свойством или парой методов get/set, вы нарушаете инкапсуляцию — внутреннее состояние выставлено наружу, и от того, что это красивое свойство, а не ужасное паблик-поле, ситуация не меняется.

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

VS>Понятно. Для библиотек согласен. Но делать всегда свойства вместо полей — паранойя. Смешно видеть у того же Фаулера в книжке по DSL вот такое:

VS>

VS>class Activity...
VS>  public string Type { get; set; }
VS>  public int Amount { get; set; }
VS>  public int Revenue { get; set; }
VS>  public string Location { get; set; }
VS>

VS>Это чисто data-класс. Никакого поведения там никогда не будет.

Так собственно какие преимущества имеет поле перед данной записью, кроме кол-ва символов?

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

VS>Ни в чем, о том и речь. Я лишь хотел сказать, что обернув приватное поле свойством или парой методов get/set, вы нарушаете инкапсуляцию — внутреннее состояние выставлено наружу, и от того, что это красивое свойство, а не ужасное паблик-поле, ситуация не меняется.
Выше приводили аргументы, чем паблик свойства лучше паблик полей. В пользу полей что-то есть?

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

VS>Смешно видеть у того же Фаулера в книжке по DSL вот такое:

Кстати, как книжка? Стоящая?

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

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

Лучше представьте куда пошлет Вас директор, когда приложение, беззвучно проглотив очередное исключение, потрет все документы на его машине, а заодно и базы с фин. отчетами. Неожиданное исключение — это однозначная смерть приложения, причем чем скорее — тем меньше вреда. Это не отменяет логгирование и какие-то меры (если возможно) по мягкому восстановлению (как в офисах, к примеру).

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

VS>Это чисто data-класс. Никакого поведения там никогда не будет.
Жили-были POCO. С public-полями. Были они "чисто data-классами".
А потом из них решили сделать self-tracking entities... и настал апокалипсис.

VS>Ни в чем, о том и речь. Я лишь хотел сказать, что обернув приватное поле свойством или парой методов get/set, вы нарушаете инкапсуляцию
ORLY?
Выставляя наружу свойство, я скрываю реализацию механизма получения/установки значения этого свойства.
Если я завтра вместо тривиального

get { return this.myProperty; }
set { this.myProperty = value; }

захочу реализовать какой-нить INotifyPropertyChanged, я его спокойно реализую и никто не помрет.
Либо изменю логику так, что свойство не будет отображаться на приватное поле 1-1.

В случае с public-полем — придется переделать контракт.

Ума не приложу, откуда такая категоричность — "никогда не будет"? Вы Б-г? Ну или хотя бы Нострадамус?

L>Кстати, как книжка? Стоящая?

Мне она показалась несколько устаревшей. Видимо, слишком долго он ее писал. Выразительность internal DSLs у него ограничивается возможностями Java, C# и Ruby (хотя последний в принципе ничего). Хотя есть целая книжка показывающая, чего можно достичь в рамках существующего языка (Boo). С другой стороны, автору удалось зацепить меня разработкой external DSL (ANTLR) Но последний у него рассмотрен скупо и местами с применением не best practics (обход AST в коде вместо создания tree grammar). В общем, если DSL для вас совершенно новая область, то прочитать стоит хотя бы потому, что это Фаулер Исли нет, то что-то новое вы там, скорее всего, не найдете. Ощущения основопологающего труда книга явно не производит.

S>Так собственно какие преимущества имеет поле перед данной записью, кроме кол-ва символов?

Поля проще. Если я вижу конструкцию, более сложную, чем она могла бы быть, у меня сразу возникает вопрос — зачем? Какие скрытые проблемы решаются? Любое усложнение кода должно быть чем-то продиктовано.

VS>>Ни в чем, о том и речь. Я лишь хотел сказать, что обернув приватное поле свойством или парой методов get/set, вы нарушаете инкапсуляцию — внутреннее состояние выставлено наружу, и от того, что это красивое свойство, а не ужасное паблик-поле, ситуация не меняется.
S>Выше приводили аргументы, чем паблик свойства лучше паблик полей. В пользу полей что-то есть?

См. выше.

HL>Ума не приложу, откуда такая категоричность — "никогда не будет"? Вы Б-г? Ну или хотя бы Нострадамус?

Если рассужать как вы, то все private члены не sealed класса надо делать protected — а вдруг захочется вызвать из наследника (я такое видел)? И virtual — а вдруг захочется переопределить? Не надо писать код, который не нужен прямо сейчас или 210% будет нужен завтра (но не послезавтра). Это не касается публичных (для команды) контрактов.

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

S>>Так собственно какие преимущества имеет поле перед данной записью, кроме кол-ва символов?

VS>Поля проще.
Чем поля проще?

VS>Если я вижу конструкцию, более сложную, чем она могла бы быть, у меня сразу возникает вопрос — зачем? Какие скрытые проблемы решаются? Любое усложнение кода должно быть чем-то продиктовано.
Преимущества свойств описаны выше. А тема простоты полей по отношению к свойствам пока не раскрыта...

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

VS>Если рассужать как вы, то все private члены не sealed класса надо делать protected — а вдруг захочется вызвать из наследника (я такое видел)? И virtual — а вдруг захочется переопределить? Не надо писать код, который не нужен прямо сейчас или 210% будет нужен завтра (но не послезавтра). Это не касается публичных (для команды) контрактов.

Нет. protected-контракт — это тоже контракт, и его надо проектировать не менее тщательно, чем публичный. Вообще суть спора мне непонятна. Кстати, обратите внимание, что генераторы клиентских частей для WCF и веб-сервисов почему-то генерят свойства, а не поля. К чему бы это, не знаете?

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

S>rsdn такой rsdn... Пока не напишешь дисклаймер на 5 страниц "не рассматриваем очень частные случаи, включаем свои мозги" — не отстанут

А чего ты удивляешься? Тут же технари все с техническим образованием. А чему нас (и, надеюсь, вас тоже) учили? "Нельзя" == "НЕЛЬЗЯ", а не "Нельзя" == "Нельзя, но ...(тут куча оговорок)". Правила математической логики, панимаешь В ней не бывает ответов вроде "да нет наверное"

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


S>Т.е. я согласен, что catch(Exception) быть не должно в идеале.

А чем плох catch(Exception) c re-throw в дебрях и логированием/сообщением об ошибке на самом верхнем уровне?

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

HL>

HL>catch (Exception e)
HL>

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

Вы сами читали-то что там пишут?
Повторяю вопрос 3-й раз за сегодня — чем плох catch (Exception) + re-throw?

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


K>"Нельзя" == "НЕЛЬЗЯ", а не "Нельзя" == "Нельзя, но ...(тут куча оговорок)".

Мы же не в этюдах, нет?
Для инженерных тем "делать надо так" == "Если не понимаешь последствий, по-другому лучше не делать. Чтобы толково объяснить — почему, придётся пройти ускоренный курс хождения по граблям с глубоким погружением в матчасть. Поскольку ты не сделал этого сам, тебе оно не надо. Так что делать надо так."

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

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


S>>Т.е. я согласен, что catch(Exception) быть не должно в идеале.

A>А чем плох catch(Exception) c re-throw в дебрях и логированием/сообщением об ошибке на самом верхнем уровне?
Что за дебри?

Если речь об этом

Автор: Angler
Дата: 12.01.11

, то оно называется log &amp; throw.

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



S>Если речь об этом

Автор: Angler
Дата: 12.01.11

, то оно называется log &amp; throw.

Да что ты цепляешься за этот log? Пусть будет так:

catch(Exception e)
            {

                //уберём за собой
                resultList.ForEach(item => item.Dispose());

                //какая нам нафиг разница, что там случилось при заполнении? 
                //пусть клиент разбирается);
                throw;
            }

Чем это плохо?

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

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

S>>Если речь об этом

Автор: Angler
Дата: 12.01.11

, то оно называется log &amp; throw.

A>Да что ты цепляешься за этот log? Пусть будет так:

A>

A>catch(Exception e)
A>            {

A>                //уберём за собой
A>                resultList.ForEach(item => item.Dispose());

A>                //какая нам нафиг разница, что там случилось при заполнении? 
A>                //пусть клиент разбирается);
A>                throw;
A>            }
A>

A>Чем это плохо?

А тут все неплохо, если не будет исключений в item.Dispose()

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

S>А тут все неплохо

А это как понимать?

Т.е. я согласен, что catch(Exception) быть не должно в идеале.

"неплохо" и "идеал" две большие разницы, не находишь? Так чем же таки catch(Exception)+rethrow хуже?

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

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

S>>А тут все неплохо

A>А это как понимать?
A>

A>Т.е. я согласен, что catch(Exception) быть не должно в идеале.

A> "неплохо" и "идеал" две большие разницы, не находишь? Так чем же таки catch(Exception)+rethrow хуже?
Я разве утверждал что catch + rethrow хуже? В чем суть претензий ко мне?

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

A>> "неплохо" и "идеал" две большие разницы, не находишь? Так чем же таки catch(Exception)+rethrow хуже?
S>Я разве утверждал что catch + rethrow хуже? В чем суть претензий ко мне?

Какие претензии? Спокойствие Мне было интересно обоснование твоей же фразы — "catch(Exception) быть не должно в идеале"

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

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

A>>> "неплохо" и "идеал" две большие разницы, не находишь? Так чем же таки catch(Exception)+rethrow хуже?
S>>Я разве утверждал что catch + rethrow хуже? В чем суть претензий ко мне?

A>Какие претензии? Спокойствие Мне было интересно обоснование твоей же фразы — "catch(Exception) быть не должно в идеале"
Боюсь, что ты пропустил контекст обсуждения. Я имел в виду catch(Exception) без проброса, как это в коде ТС.

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

S>Мы же не в этюдах, нет?
S>Для инженерных тем "делать надо так" == "Если не понимаешь последствий, по-другому лучше не делать. Чтобы толково объяснить — почему, придётся пройти ускоренный курс хождения по граблям с глубоким погружением в матчасть. Поскольку ты не сделал этого сам, тебе оно не надо. Так что делать надо так."

Очень странное определение "делать надо так". Я лично эту фразу воспринимаю буквально, а потому, если мой опыт и/или мои знания свидетельствуют об обратном, то я стараюсь обратить внимание автора на этот факт.

Вот, только что, написал 12 SQL-запросов в mysql редакторе, нажимаю "Выполнить", прога немного думает, затем сообщает что "Непредвиденная ошибка" и закрывается. Ну а я пишу запросы снова..
Нет уж, лучше бы он мне просто бы ошибку выдал и не закрывался!

	Скрытый текст
	Разработчики наверное тоже гайдлайнов начитались...

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

__>Лучше представьте куда пошлет Вас директор, когда приложение, беззвучно проглотив очередное исключение, потрет все документы на его машине, а заодно и базы с фин. отчетами.

Как это надо писать код, чтобы непредвиденное исключение удаляло все документы на машины? Ты долго этому обучался?

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

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

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

HL>

HL>catThread.Abort()
HL>Thread.CurrentThread.Abort()
HL>

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

В дополнение: http://tdanecker.blogspot.com/2007/08/do-never-ever-use-threadabort.html
Вызов Thread.Abort() может привести к неприятным последствиям.
Я бы рекомендовал использовать CancellationToken.