Constructors and Methods on POCO classes with the IRepository Pattern

Question

Is it okay to have a constructor or other non database accessing methods on POCO classes. For example when passing a view model to a controller.

Controller:

public ActionResult SomeMethod(SomeViewModel model) 
{
    var entity = new SomePocoClasse(model);
    // ... then save entity to database

    return SomeActionResult
}

Entity:

public SomeClass() {}

public SomeClass(SomeViewModel model) 
{
    // create itself based on values in model
}

public void Update(SomeViewModel model)
{
    // Update itself base on values in model
}

The first entity constructor is for entity framework or regular creation

var entity = new entity 
{
    // set up properties
};

The second is for creation from SomeViewModel

var entity = new entity(SomeViewModel);

The method is for updating itself from SomeViewModel

var entity = SomeIRepository.Get(id);
entity.Update(SomeViewModel);

Or is the above bad practice which should go some where else.

Answer 1

Yes and no. In general, it's not necessarily bad practice to have a constructor on a POCO. There's any number of reasons why you might want or need that. However, you need to ensure that you maintain a parameterless constructor as well, or you'll cause issues with things like EF which won't know how to properly initialize your POCO class otherwise.

That said, what you're doing here is not good practice. You haven't provided a ton of code, but it appears that what you're doing is passing in the view model to the POCO constructor to set the properties on the POCO with those values. Rather, what you should be doing is pulling the entity fresh from the database and then mapping over any relevant properties on your view model to that entity instance. I supposed what you're doing could be fine solely when creating a new entity, but that means having two separate ways of populating your POCO class with values depending on whether you're creating or editing. That increases complexity and complexity means higher maintenance costs.

Instead, you should either use a library like AutoMapper or create a utility class to handle the mapping:

public static class SomePocoMapper
{
    public static SomePoco Map(SomeViewModel model)
    {
        return Map(model, null);
    }

    public static SomePoco Map(SomeViewModel model, SomePoco entity)
    {
        entity = entity ?? new SomePoco();
        // map over property values;
        return entity;
    }
}

Then in your create action:

var entity = SomePocoMapper.Map(model);

And in your edit action:

var entity = // get entity from database
SomePocoMapper.Map(model, entity);