среда, 9 января 2013 г.

8 наиболее распространенных ошибок C# программистов

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

Я иногда почитываю dzone и периодически нахожу там довольно интересные статьи, и вот вчера, в разделе "Popular on DZone" наткнулся на любопытную статью под названием “8 Most common mistakes C# developers make”.

Я предлагаю вначале полистать вам ее самому, а уже потом смотреть мои комментарии.

.

.

.

.

.

.

Итак, в статье представлены 8 наиболее распространенных ошибок, которые допускают программисты на языке C#, давайте рассмотрим их более подробно.

1. String concatenation instead of StringBuilder

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

Буквальное следование подобным советам и параноидальная боязнь конкатенации строк приводит к тому, что StringBuilder начинают использовать даже для объединения нескольких строк, что приводит к падению читабельности без какого-либо выигрыша в производительности.

2. LINQ - 'Where' with 'First' instead of FirstOrDefault

Совет сводится к тому, чтобы нужно использовать метод FirstOrDefault вместо метода First или, в крайнем случае, использовать метод First, принимающий предикат, вместо использования пары методов Where/First.

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

Говорить о том, что следует использовать FirstOrDefault вместо First все равно, что советовать использовать ‘as’ вместо приведения типов ‘(T)’. У этих методов разная семантика, поэтому если мы знаем, что отсутствие определенного значения в последовательности является ошибкой, то метод First как раз то, что нам нужно.

Если же главной проблемой по мнению автора, заключается в использовании пары методов Where/First, то это другой вопрос. Во-первых, это совет из разряда микрооптимизаций; при написании подобного кода мы должны сосредоточиться прежде всего на его читабельности, а уже потом мерять производительность с помощью профилирования. Во-вторых, совет не совсем корректный, поскольку использование Where с последующим вызовом First обычно даже более эффективно по сравнению с вызовом метода First, принимающего предикат.

Все дело в том, что в методе Where используются специализированные итераторы для некоторых типов коллекций. Так, при использовании списков или массивов именно пара Where/First будет эффективнее по сравнению с методом First, принимающим делегат. Если же специализированного итератора нет, то эффективность обоих вариантов будет одинаковой.

// Список содержит довольно много элементов
var list = new List<int>() { };
// Первый вариант более эффективный
var value1 = list.Where(x => x == 42).FirstOrDefault();
var value2 = list.FirstOrDefault(x => x == 42);

В общем, совет достаточно бестолковый, да еще и нарушает Single Responsibility Principle, поскольку описывает две проблемы одновременно, при этом не описывает ни одну из них достаточно полно.

3. Casting by means of '(T)' instead of 'as (T)' when possibly not castable

Это совет, как и предыдущий, очень спорен. Он сводится к тому, что мы должны предпочесть использование преобразования типов с помощью ‘as’ вместо стандартного преобразования вида ‘(T)’, аргументируя это тем, что ‘as’ более эффективный и может применяться даже тогда, когда стандартное преобразование типов невозможно.

Во-первых, существует очень мало случаев, когда мы можем использовать преобразование типов с помощью as, но не можем использовать преобразование типов с помощью ‘(T)’: например, при использовании обобщений (о чем совсем недавно писал Эрик Липперт). Поскольку это явно крайний случай, на его основе не следует делать выводы об использовании одного способа преобразования типов по сравнению с другим.

Во-вторых, мы, опять таки, не можем говорить о корректности одного варианта преобразования типов над другим, поскольку они предназначены для разных целей. Использование ‘(T)’ говорит читателю нашего кода, что вы «знаете» о конкретном типе объекта, и если он не совпадает, то это является ошибкой. ‘as’ является безопасным преобразованием типов, при этом у нас появляется возможность “справиться” с неизвестным типом самостоятельно.

Я бы, например, дал противоположный совет: подумайте об использовании ‘(T)’ вместо ‘as, поскольку очень часто используется «безопасное» приведение типов без последующей проверки результата на null.

EventArgs e = null;
var a = e as CustomEventArgs;
Console.WriteLine(a.Value);

Наш код должен максимально четко передавать намерения программиста и использование правильных методов (First vs FirstOrDefault) или языковых конструкций (‘as’ vs ‘(T)’) являются ключевым механизмом в этом деле. При этом я бы советовал делать контракты максимально жесткими и не захламлять свой код десятками проверок на null там, где делать этого не следует.

4. Not using mapping for rewriting properties

Наличие или отсутствие рекомендуемых мапперов является вопросом дизайна (причем достаточно субъективным) и может быть актуальным лишь для определенного класса задач. Отсутствие мапперов само по себе не является проблемой, если это не приводит к дублированию кода или падению сопровождаемости. Вот нарушение принципа DRY (Don’t Repeat Yourself) и интенсивное использование паттерна “Copy-Paste” являются более общей проблемой, на которую следует обратить внимание.

Мне не нравятся подобные советы из-за их субъективности и узости применения. Подобных частных случаев слишком много, чтобы стараться их запомнить; значительно эффективнее следовать более общим принципам проектирования и стараться использовать свои знания повторно.

5. Incorrect exception re-throwing

Один из немногих советов, к которому у меня нет никаких вопросов.

6. Not using ‘using’ for objects disposal

Это второй адекватный совет, пусть и из серии КО.

7. Using ‘foreach’ instead of ‘for’ for anything else than collections

Мне казалось, что споры типа for vs foreach уже давно в прошлом, поэтому я искренне удивлен увидеть подобный совет в статье 2013 (!) года, аргументируя совершенно невнятной статьей 2004-го года.

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

Мне не хочется даже обсуждать этот совет, поскольку в нем нет никакого рационального звена. Но меня весьма пугает судьба компании Goyello, чей project manager раздает подобные советы.

8. Retrieving or saving data to DB in more than 1 call

Общеизвестные принципы проектирования, такие как связность и связанность (coupling и cohesion) можно применять не только при проектировании классов и модулей, но и при написании технических статей.

У этой статьи явная проблема со связностью (она у нее очень низкая), поскольку советы слабо коррелируют между собой. Совет о правильной работой с базами данных выглядит подозрительным среди советов об обработке исключений и циклами foreach. И хотя рекомендация сама по себе выглядит разумной она слишком общая, чтобы можно было ее каким-то образом применять на практике.

Заключение

Печально видеть в 2013 году советы из разряда «используйте for вместо foreach», особенно на вполне адекватных сайтах, типа dzone. Основная цель моей статьи в том, чтобы вы думали, а не принимали советы на веру (да, и мои советы в том числе).

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

51 комментарий:

  1. По пункту 1: для сложения 2-3 строк SB приведет к оверхеду, т.к. вот такой кусок: a + b + c будет преобразован компилятором в string.Concat(), а Concat заранее выделяет память, нужную для результирующей строки и просто кпирует туда все, что нужно.

    ОтветитьУдалить
  2. По пункту 6: есть у меня подозрение, что в контексте WinRT и CLR Projections паттерн using стал сильно выхолощенным.

    ОтветитьУдалить
  3. @Андрей: да, спасибо. Именно поэтому забавно смотреть на использование StringBuilder-ов в подобных случаях.

    ОтветитьУдалить
  4. Андрей, а можно по второму моменту более подробно?

    ОтветитьУдалить
  5. По поводу FirstOrDefault():
    Это тоже из серии "Да, Кэп", но у нас разрешается использовать Firstы только для тестов, а для кода пишем Selectы, разумеется есть случаи, когда нужен именно First, но чаще бизнес логика требует получить все же единственный, а не первый попавшийся.

    ОтветитьУдалить
  6. @denis: если нужен именно единственный элемент, то более подходящим является Single или SingleOrDefault, если элемента может и не быть.

    З.Ы. Это и правда совет в стиле "Кэп, спасибо". Есть документация, которая четко говорит, для чего нужен этот метод. Так что такой совет выглядит немного странным.

    ОтветитьУдалить
    Ответы
    1. Этот комментарий был удален автором.

      Удалить
    2. Хорошо, а если на проекте за целостность отвечает база данных, и я знаю, что там 1 объект, мне кажется выгодней использовать имено First. Хотя наверное лучше сделать перегрузку аля I_Am_Single_And_I_Know_It который будет First.

      Удалить
    3. Если вы знаете, что в базе есть 1 и только один объект и его отсутствие означает ошибку и рассогласование системы, то тем более нужно вызывать метод Single. Ведь метод Single как раз и означает I_Am_Single_And_I_Know_It.

      Удалить
    4. Да, но я имею ввиду что в базе НЕ может быть два таких элемента по PK или уникальности.

      Удалить
    5. Если НЕ может быть, тогда именно SIngle будет самым правильным решением.

      Удалить
  7. Не понял насчёт using.

    Почему не использовать?

    ОтветитьУдалить
  8. Автор статейки, похоже, какие-то баллы пытается заработать - для диссертиции, для аттестации на MVP и т.п.

    ОтветитьУдалить
  9. @aaaaaaaaa: тут имеется ввиду, что не использование using является одной из типовых ошибок.

    @Konstantin: да, похоже. Любопытно то, что она попала в популярные на dzone (9 плюсов, 1 минус), вот что странно.

    ОтветитьУдалить
  10. Поподробнее насчет WinRT: семантика IClosable там несколько отличается от семантики IDisposable в старом добром CLR. Отличается за счет обязательной асинхронности всех I/O-операций. WinRT-шный Close не делает I/O-операций => могут быть потери данных, если ЯВНО не вызывать CloseAsync. Как-то так.

    ОтветитьУдалить
  11. 2 Konstantin >> для аттестации на MVP
    Можете поподробнее про то как эта аттестация проходит?

    ОтветитьУдалить
  12. Это антисоветы какие-то.

    1, 2, 7 - preemptive optimization is root of all evil.

    Совет номер 3 вообще вредительство.

    ОтветитьУдалить
  13. имхо, совету 8 тут все же место, хоть ошибка и гораздо шире языка c#.
    очень большое количество решений, что я встречал, вносило как раз подобную проблему (скорее архитектурную и относящуюся не только к БД). еще одно следствие подобных ошибок - рассказы от кандидатов на интервью, что nhibernate/ef/remoting/wcf/... - плохая технология/библиотека/...

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

    ОтветитьУдалить
  14. Больше показалось, что пост проплачен dzone :о)) ибо сама статья [за своей ценностью] забывается сразу, а название ресурса запоминается :о)

    Вообще, и читать до конца, и тем более публично коментировать такие… явно необдуманные и тем более неактуальные материалы, как статья ув. Pawel Bejger - ну… это как конфетку у мальчишки отобрать… :о) Таких материалов, к сожалению, очень много.

    ОтветитьУдалить
  15. @Andrii Butenko: для получения статуса MVP требуется заполнение определенного документика, который подтверждает заслуги за определенный период (обычно, за год).

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

    Потом на основе этих данных принимается решение, достаточно этого для получения/подтверждения статуса MVP или нет.

    Как-то так. Если чего, спрашивай, расскажу более подробно.

    ОтветитьУдалить
  16. @mihasic: я согласен, что проблема толковая, просто описана она бестолковым образом:) Да и вообще, подобной проблеме можно отдельный пост посвятить, а не отделываться парой фраз.

    @Vyacheslav: согласен. Но, сори, не сдержался:)

    ОтветитьУдалить
  17. А разве компилятор не оптимизирует foreach, преобразуя его в for, где это возможно?

    ОтветитьУдалить
  18. Алексей> А разве компилятор не оптимизирует foreach, преобразуя его в for, где это возможно?

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

    ОтветитьУдалить
  19. Да, статья очень своеобразная. У меня однажды было собеседование: дали человеку проблемный код, чтобы ОН рассказал, что в нем плохого.
    Там был и switch c типом объекта в качестве параметра, и явный кандидат на DI, и кривой проброс исключения, и отсутствие using...
    Из всего этого великолепия говнокода кандидат уверенно выбрал foreach, как требующий обязательную замену на for во имя эффективности!

    ОтветитьУдалить
  20. Для ценителей вот ещё одно творение, не хуже.

    http://blog.xamarin.com/eight-reasons-c-sharp-is-the-best-language-for-mobile-development/

    ОтветитьУдалить
  21. Про 7) на SO есть - http://stackoverflow.com/questions/365615/in-net-which-loop-runs-faster-for-or-foreach

    Вообще статья на дзоне напомнила анекдот с РСДНа о том, что в программировании нет догм и все зависит от задачи http://www.rsdn.ru/forum/info/FAQ.philosophy.kungfu

    ОтветитьУдалить
  22. @Sergey Teplyakov
    Ну я как бе уже 4-ый год MVP. Просто номинация/реноминация и аттестация - немного разные вещи...

    http://mvp.microsoft.com/en-US/findanmvp/Pages/profile.aspx?MVPID=c40c68b0-8bdd-4eba-a02a-1ff26f6dad3b

    ОтветитьУдалить
  23. Есть нюанс. Если у вас в системе обрабатывается параллельно очень-очень много однаковых объектов (HighLoad) и для этих объектов применяется foreach (да не по одному разу в процессе оработки), то у вас либо сборщик мусора захлебнется, либо программа захлебнется. Поэтому старый добрый for(...;...;...){...} будет куда лучьше...

    ОтветитьУдалить
  24. @Andrii: понял. Кстати, поздравляю с продлением звания в январе!

    @Kolobok: не понял, какое отношение использование foreach к нагрузке на сборку мусора.

    Ведь именно для избегания подобных проблем все итераторы стандартных коллекций являются струкутрами.

    Откуда там возмутся дополнительные аллокации объектов?

    ОтветитьУдалить
  25. По поводу First or FirstOrDefault. Впрочем как и Single or SingleOrDefault. Как по мне - у них разные паттерны использования. First - когда я точно знаю, что элемент в коллекции есть( имхо, не самая распространненая возможность), FirstOrDefault - когда когда коллекция может быть пустой - а вот это - наиболее распространнеый случай. Если не говорить о проверке Count коллекции. В случае с предикатом - все еще хуже - неопределенностей больше. Мне удобнее проверить возращаемое значение на null(default(T)), чем ловить InvalidOperationException от First. Представь, есть очередь - в нее вставляет объекты три генератора объекты разных типов (асинхронно). Consumer - хочет выбрать объект определенного типа из очереди для обработки. FirstOrDefault(item => item.RequiredType == REQUIRED_TYPE) вернет тебе null если объекта нужного типа нет в очереди, и объект если есть. Для First - пришлось бы городить обработку exceptions. В общем, я бы сказал так - они не заменяют друг друга. У каждого своя ниша по использованию.

    ОтветитьУдалить
  26. Проблема с foreach актуально для Java но не для .Net.

    ОтветитьУдалить
  27. Предпочитать "as T" вместо "(T)" ... и это пишет инсайдер MS.

    ОтветитьУдалить
  28. @Achmedzhanov Nail пишет...
    Предпочитать "as T" вместо "(T)" ... и это пишет инсайдер MS.

    Что за инсайдер об этом пишет?

    ОтветитьУдалить
  29. В статье подпись "by PAWEL BEJGER in C#,MICROSOFT,SOFTWARE DEVELOPMENT", я грешным делом подумал PAWEL BEJGER работает в MS :)

    ОтветитьУдалить
  30. @Achmedzhanov Nail Понял. Хотя к любым советам нужно относиться со здоровым прагматизмом. Как минимум, совет можно неправильно понять, например, из-за неверно понятного контекста.

    ОтветитьУдалить
  31. На счет StringBuilder и Concat: при оптимизации производительности такие вещи как правило не являются узким местом. Узкие места содержатся как правило в дизайне, когда надо сделать вместо единичной операции пакетную, кеширование и т.д., эта замена даст серьезный прирост производительности.
    Если же есть серьезная разница от использования string.builder и concat, то тогда также будет полезным заменить свойства на поля, избегать дополнительных вызовов методов, использовать вместо коллекций массивы, где-то использовать структуры. Это будет уже совсем другой код и оптимизация производительности конкретных методов и алгоритмов.
    По этому такой одинокий совет на счет SB и string.Concat мимо кассы. Примерно тоже самое относится к упаковке/распаковке, если конечно это не RTD.

    ОтветитьУдалить
  32. На счет StringBuilder и Concat: при оптимизации производительности такие вещи как правило не являются узким местом. Узкие места содержатся как правило в дизайне, когда надо сделать вместо единичной операции пакетную, кеширование и т.д., эта замена даст серьезный прирост производительности.
    Если же есть серьезная разница от использования string.builder и concat, то тогда также будет полезным заменить свойства на поля, избегать дополнительных вызовов методов, использовать вместо коллекций массивы, где-то использовать структуры. Это будет уже совсем другой код и оптимизация производительности конкретных методов и алгоритмов.
    По этому такой одинокий совет на счет SB и string.Concat мимо кассы. Примерно тоже самое относится к упаковке/распаковке, если конечно это не RTD.

    ОтветитьУдалить
  33. @Слава: не нужно путать преждевременную оптимизацию с заведомой пессимизацией. Как раз я против "одинокого" совета, а за советы с контекстом: в циклах стринг билдеру - самое оно, в других случаях - нет. Да и я вообще не пойму о какой "кассе" идет речь: об исходном совете или моем варианте;)

    ОтветитьУдалить
  34. Этот комментарий был удален автором.

    ОтветитьУдалить
  35. Об исходном совете. Когда реально сталкиваешься с оптимизацией, то можно было бы плясать от счастья, если заменить str1+str2 на string.Concat(). В реальности если будет текст из 10ка мегабайт, то очевидно, что там должен быть StringBuilder. А если 300-500 строк, по 50 символов, то выигрыш очень смешной, и замена сложения строк, на sb погоды не сделает.
    По этому когда пишут про оптимизацию производительности и StringBuilder, то это про то как надо работать с большим объемом текста. При реальной оптимизации производительности, возникают совершенно другие проблемы, нежели чем замена конкатенации строк :)

    ОтветитьУдалить
  36. @Слава: для создания сложного текста я использую StringBuilder не столько ради эффективности, сколько ради выразительности. Fluent syntax StringBuilder-а мне кажется более подходящим, нежели ручная конкатенация строк.

    В любом случае, не стоит путать преждевременную оптимизацию с заведомой пессимизацией:)

    ОтветитьУдалить
  37. >>>Есть нюанс. Если у вас в системе обрабатывается параллельно очень-очень много однаковых объектов (HighLoad) и для этих объектов применяется foreach (да не по одному разу в процессе оработки), то у вас либо сборщик мусора захлебнется, либо программа захлебнется. Поэтому старый добрый for(...;...;...){...} будет куда лучьше...


    на счет foreach, буквально на днях столкнулся с тем, что в java при большом количестве таких проходом(фор-ичем) vm, действительно захлебывалась и падала с out of memory. Разговор идет конкретно о линкедлисте, встроеном, не самописном. Переход на обычный for исправил проблему сразу же. Так что возможно в шарпах существует такая же проблема, хотя не проверял. Это я к тому, что @Kolobok, возможно был не так далек от истины.

    ОтветитьУдалить
  38. @Павел: я бы не рекомендовал проводить прямые параллели между Java (в которой вообще нет кастомных объектов, аллоцируемых на стеке) и C#.
    В .NET *гарантированно* не будет никаких аллокаций в куче при переборе элементов с помощью foreach.
    Подробности тут - Duck typing или “так ли прост старина foreach?”.

    ОтветитьУдалить
  39. Этот комментарий был удален автором.

    ОтветитьУдалить
  40. На счет StringBuilder-а, почему то многие забывает об возмонжости указать размер создаваемоего массива. К примеру если у нас есть массив строк, в котором мы знаем что суммарно (обычно) 10к - 20к символов, то почему бы не создать SB с размером 20к. Увы, давно мне рассказывали что если ты не знаешь 100% сколько будет символов это бесмысленно, но почему? Лучше выделить память 1-2 раза, чем 20-30 раз. Я прав?

    ОтветитьУдалить
    Ответы
    1. Денис, чтобы стринг билдер "перевыделил" память 30 раз, то он должен работать со строкой размером 1Гб;) Ведь его размер растет нелинейно.

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

      Удалить
    2. На самом деле, сейчас как раз уже начинается оптимизация и я просто осматриваюсь что можно и нужно оптимизировать.

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

      Удалить
    3. Денис, если уже начинается оптимизация, то просматривать нужно не глазами, а с помощью профайлера. Без него, делов не будет, и даже хуже можно сделать.

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

      Можно, а смысл? Если это играет роль, то нужен не логгер а профайлер, а если это метод, который вызывается раз в день и не влияет на эффективность приложения, то зачем на него время тратить?

      Удалить