0
\$\begingroup\$

I was reading about anemic data models and rich domain models in DDD. I don't want to follow DDD completely but take rather pragmatic approach and just take some concepts out of it because clean ddd seems like an overkill. What I'm building is Web API (using ASP.NET Core and Entity Framework Core).

I don't want to maintain separate DTOs just for serializing and deserializing my models.

And I want to make most of the things like service layer and my controllers generic. Because it's mostly CRUD and logic is repeating - a lot of boilerplate.

I'm not planning to use any other ORM other than EF Core.

EF Core allows mapping to backing fields:

https://ardalis.com/encapsulated-collections-in-entity-framework-core https://technet.microsoft.com/en-us/mt842503.aspx

Is it fine if I will design my models like this and at the same time use them as my DTOs for serialization and deserialization?

public class Player
{
    [JsonIgnore]
    public int Id { get; set; }

    [NotMapped, Required]
    public PlayerInfo ActiveInfo { get; set; } // setter must be public to allow deserialization (or use custom resolver for json net)

    [JsonIgnore]
    public ICollection<PlayerInfo> PlayerInfos { get; set; } = new HashSet<PlayerInfo>();

    private Player()
    {
    }

    public Player(PlayerInfo info)
    {
        info.IsActive = true;
        PlayerInfos.Add(info);
    }

    public void SetActiveInfo(PlayerInfo playerInfo)
    {
        var currentlyActiveInfo = PlayerInfos.SingleOrDefault(info => info.IsActive);
        if (currentlyActiveInfo != null)
        {
            currentlyActiveInfo.IsActive = false;
        }
        playerInfo.IsActive = true;
        PlayerInfos.Add(playerInfo);

        ActiveInfo = playerInfo;
    }
}

This is controller:

public class PlayerController
{
    private readonly DbContext _dbContext { get; set; }
    private readonly DbSet<Player> _players { get; set; }

    public PlayerController(DbContext dbContext)
    {
        _dbContext = dbContext;
        _players = dbContext.Set<Player>();
    }

    // Create new player
    [HttpPost]
    public ActionResult<Player> Post([FromBody] Player player)
    {
        if (!ModelState.IsValid)
        {
           //Handle validation error
        }
        // No service layer for the sake of simplicity.
        player.SetActiveInfo(player.ActiveInfo); // will use _activeInfo
        _players.Add(player); // begins tracking entity 
        if (_dbContext.Save() == 0)
        {
            // handle error

        }
        return player;
    }
}
\$\endgroup\$

closed as off-topic by Dannnno, Graipher, Stephen Rauch, 200_success, Jesse C. Slicer May 14 '18 at 22:01

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Dannnno, Graipher, Stephen Rauch, 200_success, Jesse C. Slicer
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ The idea of CodeReview is to review working code. I doubt this is actually working because I don't see any way to get _playerInfos loaded in EF core. Is this actually working as intended? If not, this question belongs on Stack Overflow. \$\endgroup\$ – Gert Arnold May 14 '18 at 11:55
  • \$\begingroup\$ I changed it to property. The question is not primarily focused on that. Thanks for your comment anyway :) \$\endgroup\$ – Konrad May 14 '18 at 11:58
  • 2
    \$\begingroup\$ Now _playerInfos is invalid in SetActiveInfo. But it's always problematic to change code significantly while it's under review, because even when it's a property I have some reservations, but obviously not the same ones. Maybe you get away with it in this case because the current answer is generic enough to cover both situations. \$\endgroup\$ – Gert Arnold May 14 '18 at 12:12
  • \$\begingroup\$ @GertArnold I guess so. I was editing in hurry and I didn't notice. Well, I must admit you're hell of a reviewer ;) But it just didn't seem like a good fit for stackoverflow. \$\endgroup\$ – Konrad May 14 '18 at 12:14
  • 5
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight May 14 '18 at 13:09
2
\$\begingroup\$

I was reading about anemic data models and rich domain models in DDD. I don't want to follow DDD completely but take rather pragmatic approach and just take some concepts out of it because clean ddd seems like an overkill.

I don't want to maintain separate DTOs just for serializing and deserializing my models.

I'm similarly pragmatic as you, and I agree with you in this situation.

KISS (keep it simple & stupid) applies here. If your application is not sufficiently large, a DTO layer abstraction is not necessary.

I have several colleagues who would disagree with this, this is an open discussion topic. Personally, I disagree with blindly implementing things just because it was needed in other projects. Before I implement something, I need to justify its existence in the current project.

I'm not planning to use any other ORM other than EF Core.

It's not so much about what you plan to use, but rather the chance of this changing in the future, and how prepared you want to be when it turns out you have to change it.


Is it fine if I will design my models like this and at the same time use them as my DTOs for serialization and deserialization?

Yes, as long as you're okay with the tighter coupling and don't start violating SRP by loading everything into your entity classes.

However, that is a separate discussion from whether or not you should use DTOs. You could just as well violate SRP on your DTO class.

\$\endgroup\$
  • 1
    \$\begingroup\$ @Konrad: For example, adding additional properties that do not map to the entity (in DB), e.g. public string FullName { get { return $"{FirstName} {LastName}"; } } That should be done either on a DTO class; or implemented by the consumer of your webservice (since the backend doesn't need the property). \$\endgroup\$ – Flater May 14 '18 at 11:56
  • 1
    \$\begingroup\$ @Konrad: Yes, DTOs are more work. But note the intention of using DTOs, it's not implemented to decrease the lines of code (it's an extra layer which brings extra work with it), but rather to allow for loose coupling between layers so that they can easily be swapped for another component. \$\endgroup\$ – Flater May 14 '18 at 11:58
  • 1
    \$\begingroup\$ @Konrad: Imagine if you sell your application, but some customers want to use an SQL database, others want to use a NoSQL database, and others want to use an XML file storage. With tight coupling, you'd need to develop 3 variants of the same codebase. With loose coupling, you develop one codebase with an easily swappable data layer (you still have three datalayer projects, but only one web service project). \$\endgroup\$ – Flater May 14 '18 at 12:17
  • 1
    \$\begingroup\$ @Konrad: Think of it this way: when you buy a desktop PC, you get to pick which components you put in it. You may want a powerful graphics card, someone else might not. Now imagine if your motherboard was designed for a specific graphics card because the developers never expected anyone to want to use a different graphics card. That's essentially what you're now deciding: "no one who uses my application will ever want to use anything other than EF Core". \$\endgroup\$ – Flater May 14 '18 at 12:22
  • 1
    \$\begingroup\$ @Konrad: The discussion is running a bit too broad now. Basically, separate your entities (classes) from your data layer. Regardless of how you store the data (SQL, NoSQL, XML), you'll always want to use a Player class. Alternatively, DTOs allow you to have data-layer specific entity classes, which then all map to the same DTO class. This means that the consumer of your web service doesn't need to change its behavior based on which storage the backend is using. \$\endgroup\$ – Flater May 14 '18 at 12:31
2
\$\begingroup\$

As I explain here, the EF model is primarily a data layer. Its classes may be suited for other roles, but their primary responsibility should always be: smooth data access.

Your question is about combining three possible roles an EF model class could play:

  1. A property bag that communicates data back and forth between the database and object-oriented code. That's obviously it's primary role.
  2. A DDD class that encapsulates business logic, i.e. data and methods working on them (the OO principle).
  3. A DTO class that communicates between some client (UI or B2B) and the business logic.

It may be perfectly valid to combine role 1 with one or both of the other roles, as long as role 1 isn't compromised in any way. Sometimes that's a thin line, but generally the Single Responsibility Principe is a good touchstone.

I can be short about combining role 1 and 3. This may be valid in simple CRUD functions, although I don't like the dependency on Json.Net in a data layer class. And who says that in each use case the same properties should be ignored? It's generally preferred to use dedicated DTOs for client traffic.

The DDD role is tougher to judge. Let me focus on your example.

The method...

public void SetActiveInfo(PlayerInfo playerInfo)
{
    var currentlyActiveInfo = PlayerInfos.SingleOrDefault(info => fo.IsActive);
    if (currentlyActiveInfo != null)
    {
        currentlyActiveInfo.IsActive = false;
    }
    playerInfo.IsActive = true;
    PlayerInfos.Add(playerInfo);
    ActiveInfo = playerInfo;
}

...relies on PlayerInfos to have been loaded. If they aren't you have to rely on lazy loading. Lazy loading is supported in EF-core since version 2.1 (currently in pre-release). For lazy loading the context must be active (not disposed) when the collection is accessed.

So there's a couple of things that may go wrong here:

  1. PlayerInfos isn't loaded and lazy loading isn't enabled: the currently active record isn't deactivated and you end up having two active records without even noticing it.
  2. PlayerInfos isn't loaded, lazy loading is enabled, but the context is disposed. Now an InvalidOperationException is thrown, like:

    An attempt was made to lazy-load navigation property 'PlayerInfos' on entity type 'PlayerProxy_3' after the associated DbContext was disposed.

To prevent these errors you'd want to ensure that whenever SetActiveInfo is to be executed PlayerInfos is eagerly loaded (using Include). Two options:

  1. Since it may not always be clear if the program control flow will end up calling the method it may seem sensible to load the collection always. But that would require Include any old time Players are loaded, even when they're not needed.
  2. Load PlayerInfos right before SetActiveInfo is going to be called (if possible).

Where's the DDD going here? For the first option the code always needs to know that Player must have loaded PlayerInfos and there may not be a single source of players: they may get loaded from the DbSet but also in Includes and navigation properties.

The second option is even worse: the caller is made responsible for Player's proper state.

In DDD you'd use a class that is guaranteed to have all data it requires to execute its methods properly. Objects of this class will be created solely by a factory that ensures their integrity. It's always safe to call their methods (Tell, don't ask).

So, to summarize, it may be possible to equip EF classes for executing business logic. It mainly depends on the extend to which it can be guaranteed that the data layer produces entities in the proper state.

\$\endgroup\$
  • 1
    \$\begingroup\$ I couldn't help answering, even after the accepted answer. I love the subject :) \$\endgroup\$ – Gert Arnold May 14 '18 at 21:01
  • \$\begingroup\$ "In DDD you'd use a class that is guaranteed to have all data it requires to execute its methods properly" - does it apply if PlayerInfos is eager loaded? \$\endgroup\$ – Konrad May 14 '18 at 21:22
  • 1
    \$\begingroup\$ It applies if you can count onPlayerInfos to be loaded always. It's the guaranteed integrity that counts. I don't think it's possible to give this assurance with an EF entity, also because EF doesn't support eager loading by configuration (like join fetch in NHibernate). \$\endgroup\$ – Gert Arnold May 15 '18 at 7:06
  • \$\begingroup\$ I think guaranteed integrity depends on implementation. If I implement it in my logic to fetch PlayerInfos then it's guaranteed to exist. \$\endgroup\$ – Konrad May 15 '18 at 7:19
  • 1
    \$\begingroup\$ Sure, if you can guarantee it, for example by having a single point of origin for Players, it's OK. \$\endgroup\$ – Gert Arnold May 15 '18 at 7:23

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