Совершенный код. Разбор выпуска №1

23 мая 2010 г.

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

Проблемы

  1. В конструкторе модели ReporterFormViewModel создается контекст БД Linq2Sql для заполнения поля Publications. Модель знает, какую ORM мы используем. При смене ORM изменения затронут даже модели из web-проекта. Это очень сильно затруднит переход к новой ORM.
  2. Модель ReporterFormViewModel нельзя протестировать с помощью модульных тестов, т.к. она обращаться напрямую к БД через Linq2Sql.
  3. Логика выборки данных для View (Model-View-Controller) происходит прямо в моделе ReporterFormViewModel и скорее всего будет дублироваться парочке других мест. Модель точно не должна знать откуда и по каким условиям выбирается список Publications, иначе это будет нарушением принципа единственности ответственности.
  4. Каждый раз, когда мы пишем new ReporterFormViewModel() происходит неявное обращение к БД.
  5. Нужно вызывать Dispose у контекста Linq2Sql, иначе возможны утечки памяти.

Улучшения

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

  1. При обращении к базе данных желательно использовать транзакцию.
  2. Мы передаем во View модель ReporterFormViewModel, при этом на post-запрос возвращаем набор разрозненных объектов. Было бы на много удобнее принимать ту же самую модель. Но это уже на усмотрение автора кода и зависит от работы с View.
  3. [HttpPost]
    public ActionResult Edit(ReporterFormViewModel model)
    {
     //...
  4. Выводить более осмысленное сообщение об ошибке в поле formReport, чем текст исключения.

Реализация

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

public interface IPublicationRepository
{
    IEnumerable<Publication> GetAll();
}

internal class PublicationRepository : IPublicationRepository
{
    public IEnumerable<Publication> GetAll()
    {
        using (var dataContext = new Linq2SqlDataContext())
        {
            return dataContext.Publications.ToList();
        }
    }
}

public interface IReportRepository
{
    Report Get(int reportId);
}

internal class ReportRepository : IReportRepository
{
    public Report Get(int reportId)
    {
        using (var dataContext = new Linq2SqlDataContext())
        {
            return dataContext.Reporters.First(x => x.Id == reportId);
        }
    }
}

Каждое обращение к БД будет происходить внутри транзакции:

public interface ITransactionFactory
{
    ITransaction Create();
}

public interface ITransaction : IDisposable
{
    // При закрытии транзакции все данные будут коммититися в БД
}

Модель будет только трансформировать данные, которые поступили из контроллера, а не заниматься их выборкой:

public class ReporterFormViewModel
{
    public ReporterFormViewModel()
    {
    }

    public ReporterFormViewModel(Report report, IEnumerable<Publication> publications)
    {
        Report = report;

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

    public IEnumerable<SelectListItem> Publications { get; set; }

    public Report Report { get; set; }
}

Каждый репозиторий будет иметь свой интерфейс, а значит контроллер перестанет быть зависим от реализаций репозиториев:

public class ReportersController : Controller
{
    private readonly IPublicationRepository publicationRepository;
    private readonly IReportRepository reportRepository;
    private readonly ITransactionFactory transactionFactory;

    public ReportersController(IReportRepository reportRepository, IPublicationRepository publicationRepository, ITransactionFactory transactionFactory)
    {
        this.reportRepository = reportRepository;
        this.publicationRepository = publicationRepository;
        this.transactionFactory = transactionFactory;
    }

    public ActionResult Edit(int id)
    {
        using (transactionFactory.Create())
        {
            Report report = reportRepository.Get(id);
            IEnumerable<Publication> publications = publicationRepository.GetAll();

            return View(new ReporterFormViewModel(report, publications));
        }
    }

    [HttpPost]
    public ActionResult Edit(int id, Report reportFromView, string saveAndExit, string saveAndStay)
    {
        using (transactionFactory.Create())
        {
            if (ModelState.IsValid)
            {
                try
                {
                    Report report = reportRepository.Get(id);

                    UpdateModel(report, "reportFromView");

                    return string.IsNullOrEmpty(saveAndExit)
                        ? RedirectToAction("Edit", new {id})
                        : RedirectToAction("Index");
                }
                catch (Exception ex)
                {
                    ModelState.AddModelError("formReport", ex.Message);
                }
            }

            IEnumerable<Publication> publications = publicationRepository.GetAll();

            var model = new ReporterFormViewModel(reportFromView, publications);

            return View(model);
        }
    }
}

Результат

Думаю, что разделение ответственности между объектами получилось очень хорошим. Также нам удалось применить принцип инверсии зависимостей. На каждую часть системы (контроллер, модель) теперь можно написать модульные тесты.

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

23 комментария:

  1. У себя в проекте (mvc2) на работе инжектирую датаконтекст linq2sql (инстанциируется ninject'ом с InRequestScope). Соответственно - создаваться создаётся, а вот диспоуз не делается (негде и некем?). Насколько это чревато?

    ОтветитьУдалить
  2. @zerkms
    Надо убедиться, что в данном случае Ninject не будет делать Dispose.

    >Насколько это чревато?
    Сколько запросов в секунду у сайта?

    ОтветитьУдалить
  3. ну и еще можно добавить что front end работает напрямую с репозиториями, без слоя бизнес-логики

    ОтветитьУдалить
  4. Что будет, если при внутри кода, обернутого в транзакцию возникнет исключительная ситуация?

    ОтветитьУдалить
  5. @hazzik
    Будет исключительная ситуация :)

    ОтветитьУдалить
  6. @Александр Бындю
    Меня интересует, что произойдет с транзакцией.

    ОтветитьУдалить
  7. @hazzik
    Это уже как реализуешь. Я вообще не привел пример реализации. Можно предположить, что она откатится.

    ОтветитьУдалить
  8. тут тоже проблемы:
    - метод GetAll вреден!
    - в методе Edit не проверяется что сущность с таким id существует
    - не хватает transaction.Commit, точнее в такой реализации транзакция не может узнать все было хорошо или плохо, т.к. у нее в любом случае вызовется Dispose

    ОтветитьУдалить
  9. @Раиль
    > метод GetAll вреден!
    Чем?

    > в методе Edit не проверяется что сущность с таким id существует
    Да, стоит проверить. Но это уже из разряда вылизывания кода.

    > не хватает transaction.Commit
    Он может быть в вызове Dispose. Но это как один из возможных вариантов.

    ОтветитьУдалить
  10. такой вывод сообщения об ошибке пользователю может привести к утеканию данных (например при ошибках из бд или файловой системы):

    catch (Exception ex)
    {
    ModelState.AddModelError("formReport", ex.Message);
    }

    ОтветитьУдалить
  11. > Он может быть в вызове Dispose. Но это как один из возможных вариантов.

    ммм... всегда Commit? а зачем тогда транзакция?

    >> метод GetAll вреден!
    >Чем?

    свои наличием. это дает возможность появляться коду типа repository.GetAll().Where(user=>user.Name=name)

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

    ОтветитьУдалить
  12. параметр string saveAndExit использует только как булев параметр, зачем тогда string ?

    ОтветитьУдалить
  13. @Раиль
    > такой вывод сообщения об ошибке пользователю может привести к утеканию данных

    Да, он мне тоже не нравится. Писал об этом.

    > ммм... всегда Commit? а зачем тогда транзакция?
    Ее же по-разному можно реализовать. Например, передать Commit или Rollback в функцию Create. Это решение не единственно правильное.

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

    ОтветитьУдалить
  14. >> ммм... всегда Commit? а зачем тогда транзакция?
    > Ее же по-разному можно реализовать. Например, передать Commit или Rollback в функцию Create. Это решение не единственно правильное.

    приведи пожалуйста пример кода. не могу понять :(

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

    Из моего опыта получается, что это требование практически всегда в итоге снимается. Но если для какой-то отдельной сущности есть такое требование (доставать все), то я бы рекомендовал не выносить этот метод в базовый Repository. :)

    ОтветитьУдалить
  15. Посмотрите на фрейворк S#arp Architecture (sharparchitecture.net)
    Там реализация DDD и разделение кода просто вылизаны

    ОтветитьУдалить
  16. @Раиль
    > приведи пожалуйста пример кода
    using(transactionFactory.Create(Transaction.Commit))
    {
    ...
    }

    Т.е. после завершения все изменения закомитить.

    > Из моего опыта получается, что это требование практически всегда в итоге снимается

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

    ОтветитьУдалить
  17. @s_tristan
    Спасибо за ссылку. Там есть что-то для текущего кода?

    ОтветитьУдалить
  18. @Александр Бындю
    >Т.е. после завершения все изменения закомитить.
    В этом случае транзакция будет закомичена, если призойдет исключительная ситуация.
    Все-таки нужен явный .Commit() перед Dispose(), а в самом Dispose() если не вызван коммит делать откат транзакции.

    ОтветитьУдалить
  19. @hazzik
    Можно и так. Тоже вариант.

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

    @hazzik и @Раиль, если видите, то можете сравнить? Плюсы и минусы подходов.

    ОтветитьУдалить
  20. @s_tristan
    Очень интересный код, есть полезные идеи. Для начала понравилась реализация TransactionAttribute для методов контроллера. Спасибо еще раз.

    ОтветитьУдалить
  21. можно привести реализацию ITransactionFactory и ITransaction

    ОтветитьУдалить
  22. @moon-pilgrim
    Для какой ORM будем реализовывать?

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

Моя книга «Антихрупкость в IT»

Как достигать результатов в IT-проектах в условиях неопределённости. Подробнее...