Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My domain model consists mostly of simple DTOs, i.e. 'Data Transfer Objects' which this article distinguishes from 'Plain Old C# Objects', like this one:

public class Settings
{
    public bool FullScreen = false;
    public int WindowWidth = 800;
    public int WindowHeight = 600;
}

I realize this model is "anemic," but it works well with the JSON serializer I use for persistence. Also, I'm still in the exploratory stage of development and I read that it's okay at this point to have a bunch of "property bags."

Other classes look less like DTOs and more like POCOs (or POJOs although I'm using C#):

public class Sprite
{
    [JsonProperty]
    private readonly string assetName;

    [JsonProperty]
    private Vector2 position;

    // other properties...

    public Sprite( string assetName, Vector2 position )
    {
        this.assetName = assetName;
        this.position = position;
    }

    public void Move( Vector2 direction )
    {
        // do move
    }

    // other methods...
}

Because this type of object needs to be persisted by the JSON serializer, the properties need either to be public or have a JsonProperty attribute. I went with the attribute in this case because I want to restrict access to its properties.

Is this a good design? Does it make sense for my "entities" (I'm not sure if that's the correct term) to look so different from one another? My gut told me no, so I tried extracting the stuff not needed by the serializer into a separate class outside of the namespace:

public class SpriteModel
{
    private readonly Sprite sprite;

    public SpriteModel( Sprite sprite )
    {
        this.sprite = sprite;
    }

    public void Move( Vector2 direction )
    {
        // do move
    }

    public void GetSprite()
    {
        return sprite;
    }

    // other methods...
}

Now the Sprite class looks more like a DTO:

public class Sprite
{
    public string AssetName;
    public Vector2 Position;
    // other properties...
}

Someone told me that my domain model is actually the SpriteModel class now and not the Sprite class itself. I'm obviously confused about what to call these things exactly, and any clarification would be appreciated.

But my main question is: which of these approaches is better and why? Are they both terrible? And in that case is there another approach that I should use instead? If I go with the second approach, then I could have SpriteModel implement ISpriteModel, and the classes that use it could be more loosely coupled, which is a nice side benefit I think. But I don't want to go that route just for a side benefit that might not actually pay off (and could just add more complexity).

share|improve this question
4  
Congratulations on asking question 12,000 on CodeReview, and a good question at that! –  rolfl Feb 15 at 20:22

1 Answer 1

I would say that one of the major differences between a DTO and a POCO is the fact that a DTO should be immutable because it is meant to be used for transferring only. See what are the differences between a POCO and a DTO in this SO question. That said, someone could suggest that your DTO implementation should be the following:

public class Settings
{
    public bool FullScreen{get; private set;}
    public int WindowWidth{get; private set;}
    public int WindowHeight{get; private set;}
    public Setting(int width, int height, bool fullScreen){
       WindowWidth = width;
       WindowHeight = height;
       FullScreen = fullScreen
    }
}

This is only if you want to be a purist in the implementation of a DTO because the usage of this is a pain, especially if your DTO class has many properties. The work around is the one you already have, although I would trade the fields for properties because properties respect the principle of encapsulation and are implemented with the same space in C# (in Java I wouldn't mind having public fields though).

About your second issue, I would say that your real problem is to isolate the domain from its representation, be it JSON or something else. So how could you separate them? I would personally use the proxy pattern:

public class Sprite
{
    public virtual string AssetName{get; set;}
    public virtual Vector2 Position{get; set;}
}

public class SpriteJson : Sprite
{
    public SpriteJson(Sprite sprite){
       AssetName = sprite.AssetName;
       Position = sprite.Position;
    }
    public SpriteJson(){}
    [JSonProperty]
    public override string AssetName{get; set;}
    [JSonProperty]
    public override Vector2 Position{get; set;}
}

It may seem that you have too much redundancy, but it does isolate the objects from their representation. You could receive a Sprite object anywhere in your code, whether it is a SpriteJson object or not.

Edit: I think I only answered your question partially, so I edited my answer to provide more details.

Leaving the representation aside, the logic associated with the object would be in your Sprite class:

public class Sprite
{
    public virtual string AssetName{get; set;}
    public virtual Vector2 Position{get; set;}
    public void Move(Vector2 direction)
    {
         // do move
    }
}

I think an object should always have the logic associated with it whenever it makes sense and is reasonable. There is no need for making a wrapper only to separate the logic (a useless abstraction, in my opinion). The question I linked also stated that POCO classes may contain logic operations. So when is it not reasonable? Imagine that your move code is highly dependent on another class and many of your objects also have their own move logic. In that case, it would be better to refactor the move code to another place so you wouldn't have code redundancy and eliminate those dependencies.

share|improve this answer
    
I like the idea of having a SpriteJson class and will probably try that. I'm using fields instead of auto-properties because I can easily specify default values. If I used auto-properties and a constructor, it would arguably involve more code for such a simple thing. However, we've had this conversation before and I think I'm just starting to get my head around your answer to that question. –  Koveras Feb 16 at 0:15
1  
The only reason that I consider that is OK to have public fields is because the DTO is meant for transfering as I said. So you won't EVER change the DTO code (unless you want to add a new field) and thus encapsulation is not a concern. So if public fields make your life easier in DTO use them. –  Bruno Costa Feb 16 at 1:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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