3

Case in point in Entity Framework, but this is a design question which has applicability to any ORM.

In the current application we have a couple of ORM data classes which do stuff not directly related to the function of that class.

One example is the Document class, which is a database representation of document stored on the network drive. It has a constructor which actually makes a physical copy of the document:

public Document(string filePath, int categoryId, User currentUser)
{
    FileInfo info = new FileInfo(filePath);
    this.Name = info.Name;
    this.Size = info.Length.ToString();

    this.CategoryId = categoryId;
    this.User = currentUser;
    this. CreatedOn = DateTime.Now;

    targetPath = ConfigurationManager.AppSettings["CampaignFolder"];
    FileHelper.CopyFile(filePath, targetPath + "\" + this.Name);
}

You can see how this would be convenient, especially if you're potentially loading files from lots of places in the code and you want them stored in a central location.

Another example is the Category class, which is related to the above. The class itself has a function that has to go back to the repository to do some additional work - so we pass one in:

public List<Document> SearchDocuments(string description, IRepository rep)
{
    if (this.Documents.Any(dc => dc.Description == description))
    {
        return this.Documents.(dc => dc.Description == description).ToList();
    }

    using (IRepository r = rep.Renew())
    {
        return r.GetDocuments(d => d.CategoryId == this.CategoryId 
                        && dc.Description == description).ToList();
    }
}

Again, you can see how this is convenient - it uses properties inherent to the object, and it's a useful place to put this function if it's called from a lot of different places.

It's also worth noting that this latter function, at least, doesn't break testability.

However, I can't help the sense that this is wrong somehow. It feels like a violation of single responsibility - these things are just for carrying data and perhaps some computed properties. But on the other hand, putting functions like this inside them removes the need for an annoying proliferation of helper classes.

Is this a bad idea? Or am I worrying over nothing?

3

I think you are correct in your concerns.

In both cases I would separate the repository functions from the object.

The document object seems particularly bad in that the logic takes place in the constructor

ie

IDocumentRepository.Save(Document doc);
IDocumentRepository.Search(string description)

This helps you achieve a number of things:

1: No dependency of the Object on the repository implementation

2: The responsibilities involved in persisting the object are encapsulated in their own repository object rather than the Domain object

  • Thanks for the answer. I do understand how I could avoid doing this if I wanted to. I'm more interested in understanding why it's a bad idea to do it in the first place? – Matt Thrower Jan 12 '16 at 15:28
1

Yes, it is not good to put externally dependent logic in ORM classes.

Ideally, an ORM's sole purpose is to provide a map from the database to the objects used by the code (ie- Object Relational Mapping)

When you muddy the responsibility of an object's purpose you're breaking the ability to swap out components from the system without breaking the other parts.

If something changes about how the files are stored you now have to go inside the entity as well as wherever the files are accessed. Two places to make changes instead of one place.

To keep the desired functionality, I would recommend setting up a service which can be aware of the Repository, Files & ORM.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.