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

Вы прислали очень много решений и замечаний к коду из выпуска №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.

[HttpPost]
public ActionResult Edit(ReporterFormViewModel model)
{
 //...

3. Выводить более осмысленное сообщение об ошибке в поле 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);
        }
    }
}

Результат

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

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