10 мая 2010 г.

Открытие рубрики «Совершенный код»

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

Я решил открыть рубрику «Совершенный код». Каждую неделю буду выкладывать проблемный код, вопросы по архитектуре или тестированию, которые вы мне присылаете. Вы можете находить проблемы в коде и описывать пути их решения. Через неделю я выложу лучшие советы со ссылкой на авторов и возможно своими дополнениями. Авторы решений, оставляйте, пожалуйста, свои контакты (сайты, блоги), чтобы я мог на вас ссылаться.

Начали!

Приложение написано на ASP.NET MVC. Мне прислали небольшой кусочек кода, связанный с редактирование отчёта. У автора возникли проблемы с изменением этого кода, написанием на него тестов и работой с моделью (M-V-C).

Контроллер ReportersController

   1:  public class ReportersController : Controller
   2:  {
   3:      Linq2SqlDataContext dataContext;
   4:   
   5:      public ReportersController() 
   6:      {
   7:          dataContext = new Linq2SqlDataContext();
   8:      }
   9:   
  10:      // ...
  11:   
  12:      // форма редактирования
  13:      public ActionResult Edit(int id) 
  14:      {
  15:          var item = dataContext.Reporters.FirstOrDefault(x => x.Id == id);
  16:          return View(new ReporterFormViewModel(item));
  17:      }
  18:   
  19:      [HttpPost]
  20:      public ActionResult Edit(int id, Reporter reporter, string saveAndExit, string saveAndStay) 
  21:      {
  22:          var item = dataContext.Reporters.FirstOrDefault(x => x.Id == id);
  23:   
  24:          if (ModelState.IsValid) 
  25:          {
  26:              try
  27:              {
  28:                  UpdateModel(item, "reporter");
  29:   
  30:                  dataContext.SubmitChanges();
  31:   
  32:                  if (saveAndExit == null)
  33:                      return RedirectToAction("Edit", new { id });
  34:   
  35:                  return RedirectToAction("Index");
  36:              }
  37:              catch (Exception ex) 
  38:              {
  39:                  ModelState.AddModelError("formReport", ex.Message);
  40:   
  41:                  return View(new ReporterFormViewModel(reporter));
  42:              }
  43:          }
  44:          
  45:          return View(new ReporterFormViewModel(reporter));
  46:      }
  47:  }

Модель ReporterFormViewModel

   1:  public class ReporterFormViewModel
   2:  {
   3:      public ReporterFormViewModel() : this(new Reporter() { CreatedDate = DateTime.Now }) 
   4:      {
   5:      }
   6:   
   7:      public ReporterFormViewModel(Reporter reporter) 
   8:      {
   9:          Reporter = reporter;
  10:   
  11:          InitCollections();
  12:      }
  13:   
  14:      private void InitCollections()
  15:      {
  16:          var dataContext = new Linq2SqlDataContext();
  17:   
  18:          Publications = dataContext.Publications.Select(x => new SelectListItem() {
  19:              Value = x.Id.ToString(),
  20:              Text = x.Name
  21:          });
  22:      }
  23:   
  24:      public IEnumerable<SelectListItem> Publications { get; set; }
  25:      
  26:      public Reporter Reporter { get; set; }
  27:  }

Тестов к коду, к сожалению, нет. Да-да, я понимаю, что делать рефакторинг без тестов довольно сложно.

Хочу заметить, что разработчики, которые могут признать, что в их коде есть проблемы, имеют все шансы стать лучше и узнать что-то новое. Надо иметь смелость, чтобы просить помощи. Поэтому в комментариях к коду приветствуется конструктив и позитив!


Разбор решения к выпуску №1

47 комментариев:

  1. Навскидку:
    1. Context не диспоузится. Использовать его в using и я бы не стал декларировать его в скопе контроллера.
    2. Убрать из модели код доступа к данным. Инициализировать все в контроллере.

    На самом деле, лично мне, было бы очень интересно увидеть на этом примере как грамотно использовать с MVC тесты.

    ОтветитьУдалить
  2. 1. Избавиться от DataContext в ReporterFormViewModel:
    Вынести логику инициализации коллекции прямо в контроллер(не всю ее часть, а только dataContext.Publications) остальную инициализацию можно перенести прямо в конструктор. Конструктор по умолчанию также убрать и вынести значения по-умолчанию в контроллер, таким образом сама вью может принять следующий вид:

    class ReporterFormViewModel {

    public ReporterFormViewModel(Reporter reporter, IEnumerable publications) {
    Reporter = reporter;
    Publications = publications.Select(x => new SelectListItem() { Value = x.Id.ToString(), Text = x.Name });
    }

    public IEnumerable Publications { get; set; }

    public Reporter Reporter { get; set; }
    }

    Т.о. мы избавимся от зависимости от контекста и сложной логики во вью.

    2. избавить контроллер от зависимости к DataContext:

    a. выделить репозиторий IReporterRepository:

    interface IReporterRepository {
    Reporter FindById(int id);
    void Save(Reporter reporter);
    }

    class L2SReporterRepository {
    public Reporter FindById(int id) {
    using(dc = new Linq2SqlDataContext()) {
    return dc.Reporters.FirstOrDefault(x => x.Id == id);
    }
    }

    public void Save(Reporter reporter) {
    using(dc = new Linq2SqlDataContext()) {
    dc.Reporters.Attach(reporter); // тут я не уверен, т.к. не работал с l2s
    dc.SubmitChanges();
    }
    }
    }

    b. выделить репозиторий IPublicationsRepository:

    interface IPublicationRepository {
    IEnumerable FindAll();
    }

    class L2SPublicationRepository {
    public IEnumerable FindAll() {
    using(dc = new Linq2SqlDataContext()) {
    return dc.Publications.ToList();
    }
    }
    }

    b. теперь наш контроллер зависит от *интерфейсов* IReporterRepository и IPublicationsRepository, который мы будем инжектировать в конструктор:

    public ReportersController(IReporterRepository reporterRepository, IPublicationsRepository publicationsRepository) {
    this.reporterRepository = reporterRepository;
    this.publicationsRepository = publicationsRepository;
    }

    Проведя эти 2 несложных рефакторинга мы сделаем контроллер польностью не зависящим от DataContext, и следовательно можно будет написать юнит-тесты.

    PS: могу по-шагам расписать все действия
    PPS: Это не окончательный рефакторинг. После него все-равно остануться проблемы, но уже и так можно жить.
    PPPS: Саня, извини, не удержался. :)))

    ОтветитьУдалить
  3. @Aleksei и @hazzik
    Описание решения проблем есть, но сами проблемы вы не показали.

    Зачем делать все эти перестановки? Какие проблемы они решат?

    ОтветитьУдалить
  4. Написал же в конце, что за проблему я решал:)

    ОтветитьУдалить
  5. @hazzik
    Я правильно понял, что изменив существующий код ты решишь одну проблему - написание модульных тестов?

    ОтветитьУдалить
  6. а ты, евангелист чёртов, работай вместо них

    ОтветитьУдалить
  7. Естественно: в коде не может быть никаких других проблем, кроме как, невозможность написать юнит-тесты *CRAZY*

    ОтветитьУдалить
  8. @hazzik

    Насколько хороша идея реализации метода Save в репозитарии, когда работаешь с UnitOfWork?

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

    ОтветитьУдалить
  9. Реализация Save() оправдана, если есть реализация Commit() в unit of work паттерне.

    ОтветитьУдалить
  10. @Илья Дубаденко
    Репозиторий ничего не должен знать про UnitOfWork, как и наоборот.

    Т.е. складываем объекты мы в любом случае в репозиторий.

    ОтветитьУдалить
  11. @Илья Дубаденко
    Я так понимаю вы про строчку dc.SubmitChanges();

    Я тут не работаю с uow, поэтому оправдано. Выделение uow можно сделать следующим этапом рефакторинга, если уж очень приспичит.

    ОтветитьУдалить
  12. @Александр Бындю

    Есть 3 подхода к реализации unit of work.
    2 из них от Fowler и 1 от Jimmy Nilson :)
    Когда ретозиторий знает об uof это избавляет от необходимости ручного взаимодействия c uow. Всю черновую работу будет на себя репозиторий.

    ОтветитьУдалить
  13. @Dmitry Sukhovilin
    А можно ссылки на все 3 реализации?

    ОтветитьУдалить
  14. It comes from .NET Domain-Driven
    Design with C# by Tim McCarthy:

    The Unit of Work needs to know what objects it should keep track of, and Martin goes on to describe two basic ways this can be accomplished:

    Caller registration — The user of the object has to remember to register with the Unit of Work.

    Object registration — The objects register themselves with the Unit of Work.

    Jimmy Nilsson describes a different approach to the Unit of Work, and that is to let the repositories
    delegate all of their work to a Unit of Work, and then the Unit of Work then makes all necessary database
    calls (or other types of resource calls) on behalf of the repositories (Nilsson, Applying Domain - Driven
    Design and Patterns, With Examples in C# and .NET , 200). One major benefit of this approach is that the
    messages sent to the Unit of Work are invisible to the consumer of the repositories, since the repositories
    are reporting what has been asked of them to the Unit of Work. This also helps promote persistence
    ignorance in the domain objects, which is what I am striving for.

    ОтветитьУдалить
  15. Fowler, Patterns of Enterprise Application Architecture , 184

    ОтветитьУдалить
  16. @Dmitry Sukhovilin
    Ну ясно, видимо они там вручную этот UoW писали.

    На много удобнее, когда репозитории ничего не знают про UoW. Все-таки это не их ответственность.

    ОтветитьУдалить
  17. @Александр Бындю

    ISesstion в NHibernate это реализация unit of work => С трудом могу представить реализацию репозитория не знающего о ISession.
    Если это не отрестстенность репоситория, то кто должен отвечать за обслюживание uow?

    ОтветитьУдалить
  18. @Dmitry Sukhovilin
    Да, в NHibernate UoW это скорее обертка над Session.

    А вы как реализовали UoW?

    ОтветитьУдалить
  19. Парни из NHibernate сделали это зя меня :)
    А вот при использовании, скажем, Microsoft EF...
    Я еще не пришел к окончательному мнению, но склоняюсь в сторону подхода от Jimmy Nilsson.
    Т.е. репозиторий трекает все изменения (CRUD) в домене.
    Если есть более удачное решение u r welcome :)

    ОтветитьУдалить
  20. @Александр Бындю

    Чем плохо то, что Repository будет зависеть от UoF? Мне кажется это нормально. Репозитории должны куда-то помещать (брать) объекты, будь это IUnitOfWork или IDatabase

    Например, http://www.handcode.ru/2010/05/repository-unit-of-work.html
    Готов выслушать критику.

    ОтветитьУдалить
  21. Дак, что? Будут другие решения-то?

    ОтветитьУдалить
  22. @hazzik
    Я думаю, что далеко не все проблемы раскрыты.

    Чем например плохо создание Linq2SqlDataContext в моделе?

    ОтветитьУдалить
  23. @Александр
    Конечно не все проблемы. Просто кроме моего решения других-то и нет:) Не буду же я в этом примере все проблемы решать...

    ОтветитьУдалить
  24. @hazzik
    Еще не вечер :)

    Ну да чем плохо создание Linq2SqlDataContext в моделе?

    ОтветитьУдалить
  25. >> Чем например плохо создание Linq2SqlDataContext в моделе?

    1. Зависимость от конкретной реализации persistence.
    Что будет если мы решим сменить L2S на EF а потом на NH? Искать все места и переписывать.
    2. Вообще зависимость от persistence.
    Что будет если нам потребуется создать Model без БД - как временный объект или для тестов?

    ОтветитьУдалить
  26. Во-первых тем, что теперь нельзя создать модель без подключения к базе данных.
    Во-вторых, при смене ОРМ, не дай бог, придется переписывать и эту часть проекта, хотя на первый взгляд с чего-бы это?
    В-третьих, это просто не тестируется без базы данных.

    В проблемы можно приписать нарушение принципов проектирования.

    ОтветитьУдалить
  27. @ankstoo
    Шикарно!

    Еще добавлю, что модель в MVC создается неявно, а обращение к БД происходит в конструкторе в функции InitCollections. В такой ситуации возможны лишние обращение к базе данных.

    Начали проблемы ковырять, хорошо... ;)

    ОтветитьУдалить
  28. @hazzik
    +1
    Подпишусь под каждым словом :)

    ОтветитьУдалить
  29. Вот мне еще интересно, что будет, когда конструкция:

    dataContext.Reporters.FirstOrDefault(x => x.Id == id)

    вернет null?

    ОтветитьУдалить
  30. Ох блин, это ж надо, linq2sql вытащить в контроллер! Мое IMHO – linq2sql classes не должны покидать пределов сборки Linq2SqlDataProvider.

    Собственно, весь код тут одна большая проблема, нарушены, по-моему все принципы создания «хорошего кода».

    ОтветитьУдалить
  31. @Sergey
    > Собственно, весь код тут одна большая проблема
    Отбросьте эмоции, Сергей :)

    Можно по пунктам, в чем заключаются проблемы?

    ОтветитьУдалить
  32. Интересно читать - при смене ORM придется....

    Вы на проектах часто ORM меняете? Начили на LLBL закончили на LINQ
    А платформу? :)

    Я хочу сказать, что нет ничего плохого если реализация persistence будет заточена под конкретный ORM.
    Ежинственный агрумент must have - это замена ORM (интерфейсов от ORM) на mock object.

    На моей практики на разу не приходилось менять ORM после пуска проекта.
    А у вас были случаи ? Если да, то в чем причина?

    ОтветитьУдалить
  33. @Dmitry Sukhovilin

    > А у вас были случаи ? Если да, то в чем причина?

    Да, я в уже в 3-4 проектах менял ORM. Причина простая. Когда проект начинается, то выбирается самая подходящая на тот момент ORM. Но со временем 1) вылезают проблемы 2) выходит более зрелый продукт и приходится отказываться от текущей ORM в пользу более совершенной.

    Вот какие были перестановки:
    * MyGeneration -> Linq2Sql
    * Linq2Sql -> EF
    * LLBLGen -> NHibernate
    * EF -> NHibernate

    В итоге пришли к тому, что сгенерированный код - это всегда неудобно. На долгосрочные проекты всем настойчиво советую NHibernate, т.к. он единственный не генерирует код.

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

    ОтветитьУдалить
  34. @Александр Бындю

    Кстати, да, вот тут описал проблемы, с которыми мы столкнулись
    http://blog.byndyu.ru/2009/12/llblgen-vs-nhibernate.html

    Хотя по началу все шло, как по маслу :) В течение года использовали LLBLGen, после этого перешли на NHibernate.

    ОтветитьУдалить
  35. >> А у вас были случаи ? Если да, то в чем причина?

    persistence - это не только ORM.
    У меня был проект, в котором, в зависимости от настроек, работа идет с одним из трех persistence: MS SQL Server, web-service или сериализованные объекты в файловой БД.

    ОтветитьУдалить
  36. @Dmitry Sukhovilin
    Никогда не говори "никогда".
    Также приходилось менять IoC контейнер.
    У нас в практике такое случается часто, потому что мы аджайлисты (кататак). Если ты делаешь agile, будь готов что в любой момент поезд твоего проекта повернет в любую сторону.

    Да, бесспорно, для небольшого проекта длиной в месяц и менее, который ты сдал и забыл вряд ли придется менять ORM или DI. Да там вообще можно на все принципы забить:))

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

    ОтветитьУдалить
  37. Я присоединяюсь к Dmitry Sukhovilin, некоторые вещи нужно выбирать "раз и насегда" для конкретного проекта. Например, тот же IoC контейнер, логгер, автомаппер, viewengine...Разумеется прежде чем выбрать что-то, нужно взвесить все за и против.

    Но(!) это вовсе не означает что нужно писать путанный код, игнорируя паттерны.

    Вообщем я за то, что не стоит гнаться за сверх универсальностью. В большинстве случаев, она просто не нужна.

    ОтветитьУдалить
  38. @Илья

    Просто когда к тебе приходит заказчик и говорит сделай мне пожалуйста двойное сальто назад, а твоя ОРМ позволяет сделать это сальто только через 3 месяца... Или ты говоришь: "ок, мы неделю меняем ОРМ" и сальто получается сделать за 2 дня.

    Смотрите сами... Раз и на всегда не получается как-то.

    ОтветитьУдалить
  39. >Но(!) это вовсе не означает что нужно писать путанный код, игнорируя паттерны.
    Нужно писать код, который работает, по возможности приводя его к уже отработанным решениям. А писать чтоб было красиво это, по меньшей мере, глупо.

    ОтветитьУдалить
  40. @hazzik
    Надо мыслить прагматично, если стоимость перехода на новую ОРМ будет дороже, чем реализация функционала гибкой смены ОРМ, конечно с учетом вероятности такого события, то тогда разумно писать сразу гибко. Стоит не забывать, что бюджет определяется толщиной кошелька заказчика, который просто так менять что-то не станет.

    >Нужно писать код, который работает...
    Истину говорите :)

    ОтветитьУдалить
  41. @Илья Дубаденко

    > некоторые вещи нужно выбирать "раз и насегда" для конкретного проекта. Например, тот же IoC контейнер, логгер, автомаппер, viewengine

    Это все тоже заменяли. Как раз последнее было с IoC контейнером. Меняли Ninject -> Windsor. Я советую делать обертку для IoC-контейнера, каким бы совершенным он ни казался.

    ОтветитьУдалить
  42. @Александр Бындю

    Сделав обертку можно утратить часть функционала.

    ОтветитьУдалить
  43. @Илья Дубаденко

    > Сделав обертку можно утратить часть функционала

    Это ты из практики своей взяли или мы про сферического коня?

    Мы сделали обертку и не потеряли функциональность.

    ОтветитьУдалить
  44. @Илья Дубаденко

    Значит эта часть функционала не нужна.

    ОтветитьУдалить
  45. Бусспорно, если заказчик будет финансировать смену ORM то будем менять.
    Но нужно будет ответить на вопрос - "А почему сразу не выбрали правильный вариант ?"
    Не все же заказчики будут вникать в проблемы девелопмента.

    ОтветитьУдалить
  46. @Dmitry Sukhovilin
    В том то и дело, что нет правильного варианта. Какая-то ORM при текущем знании о системе кажется идеальной. Через год система становится совершенно другой и в этом свете другая ORM становится идеальной.

    Видимо это издержки Agile. Движение никогда не прекращается. Мы не можем тешить себя надеждой "сразу выбрать правильный вариант".

    ОтветитьУдалить