1

I have the following class:

    public class SectionViewModel
    {
        private static EFModels.VTSEntities _db = new EFModels.VTSEntities();
        public int ID { get; set; }
        public string SectionName { get; set; }
        public bool Active { get; set; }
        public string SiteName { get; set; }
   }

I want to select one element from _db.Sections and fill object of this class. I can do it like it:

    public SectionViewModel(int ID)
    {
        var s = (from i in _db.Sections
                 where i.ID == ID
                 select new SectionViewModel()
                 {
                     ID = i.ID,
                     SectionName = i.SectionName,
                     Active = i.Active,
                     SiteName = i.Site.SiteName
                 }).FirstOrDefault();
        ID = s.ID;
        SectionName = s.SectionName;
        Active = s.Active;
    }

It works, but when count of fields is tens, code is huge. I would like to write something similar

    // IT DOES NOT WORK, ONLY EXAMPLE
    public SectionViewModel(int ID)
    {
        this = (from i in _db.Sections
                 where i.ID == ID
                 select new SectionViewModel()
                 {
                     ID = i.ID,
                     SectionName = i.SectionName,
                     Active = i.Active,
                     SiteName = i.Site.SiteName
                 }).FirstOrDefault();
    }

ADDED:

Create SectionViewModel object (it's a View model class):

    public ActionResult SectionForm(int? id)
    {
        SectionViewModel model = new SectionViewModel(id);
        return View(model);
    }

but, of course, it's impossible because "this" is available only for read. Any way to do it?

Oleg Sh
  • 8,496
  • 17
  • 89
  • 159
  • @Gert Arnold, I have added – Oleg Sh Mar 24 '15 at 22:14
  • If I understand your problem correctly, this is why products like [Automapper](http://automapper.org/) exist. – Erik Philips Mar 24 '15 at 22:20
  • 1
    I'd recommend you read about how to work with MVC + EF. Your code is an dense collection of wrong patterns apparently inspired by faint echoes of past programming traditions. Back to the drawing board. – Gert Arnold Mar 24 '15 at 23:31
  • I have read tons of such articles. Could you describe why my approach is wrong? I just want to fill my View model in constructor if it's necessary. Which pattern is broken? – Oleg Sh Mar 24 '15 at 23:46

1 Answers1

2

Edit

OK, so you construct a SectionViewModel, not a Section, from a Section. That makes some difference, but still many remarks from my original answer apply.

A better way to do this is

public ActionResult SectionForm(int id)
{
    Section section = this._context.Sections.Find(id);
    SectionViewModel model = .... // Mapping code
    return View(model);
}

This part // Mapping code can be anything that copies properties from section to model. You could use AutoMapper.

The reason not to do this in SectionViewModel's constructor is that there shouldn't be a static context there in the first place. You could create and dispose a context in stead. But who says that SectionViewModels are always constructed individually? Maybe in another method you will return a list of them. Creating each model separately would be highly inefficient. AutoMapper's Project().To approach would be appropriate there.


Original text

It's absurd to construct an object in a constructor (which is what happens by the part var s = (...).FirstOrDefault()) and then copy its properties to the owner of the constructor, which is the same type, Section. It's even more absurd that in the query you also construct a Section from a section. So after running the statement...

Section model = new Section(id);

...you have constructed three identical Sections of which the last one is finally used:

Section 1: from i in _db.Sections where i.ID == ID select i.
Section 2: select new Section() {...}
Section 3: Section(int ID)

And EF doesn't even allow a statement like

from i in _db.Sections
select new Section() {...}

It will tell you that you can't construct an entity in an LINQ-to-Entities query.

But removing this select new Section() {...} is not even the beginning of a sound refactoring. The whole construct is absurd.

Another bad practice is to have a static context. Contexts are designed to have a short lifespan, because they cache each entity they fetch from the database. A static context is a memory leak.

The way to achieve what you want is simply...

public ActionResult SectionForm(int id)
{
    Section model = this._context.Sections.Find(id);
    return View(model);
}

...where this._context is an instance of your VTSEntities that is created per controller instance (or injected into it by an Inversion of Control container).

Your programming style is faintly reminiscent of Active Record, a pattern that doesn't mix well with EF's repository/unit of work pattern (where DbSet is a repo and a DbContext is a UoW). It also breaks the persistance ignorance principle that EF is built on. It breaks query composability and it easily causes n + 1 queries.

Community
  • 1
  • 1
Gert Arnold
  • 105,341
  • 31
  • 202
  • 291
  • My Section class is View Model class, not EF class! So, your example is not appropriate. I will remove Section class to SectionViewModel in my question for more clear! – Oleg Sh Mar 25 '15 at 22:43
  • and using EF model class for View is very bad approach (which I can see in your controller method). EF model is EF model, View model is View model! – Oleg Sh Mar 25 '15 at 22:46
  • Please see the revised answer. – Gert Arnold Mar 26 '15 at 10:49
  • if I would like from 2 or more entities (i.e. SectionViewModel is filled from Sections and one parameter from Sites), your approach is unappropriate – Oleg Sh Mar 26 '15 at 23:07
  • I just point a situation, why your approach can't be good. You propose to get an object of model class and then copy to object of view model class. – Oleg Sh Mar 27 '15 at 00:39
  • The point of this Q&A is the place where you construct view model. How and what to create is secondary. Of course you can project *any* query to *any* view model. You can project to view models *containing* view models, which is another reason not to do the query in a VM's own constructor. – Gert Arnold Mar 27 '15 at 07:10