5
\$\begingroup\$

I have an abstract class Entity in my code, which requires a reference to an instance of EntityData to operate. (EntityData provides basic information about the type of the entity).
In the design of my program, every type of entity would have its own class that extends Entity, and thus would have to provide EntityData that would describe and be shared between all of the instances of that that entity class.
EntityData itself is an immutable class that is built like a multiton (similar to Java's enum). It has a private constructor, and its instances are revealed as readonly static fields.

An example of how EntityData would look:

sealed class EntityData
{
    public static readonly EntityData Table = new EntityData(250, 60);
    public static readonly EntityData Chair = new EntityData(120, 25);

    public readonly int Cost;
    public readonly int Weight;

    private EntityData(int cost, int weight)
    {
        Weight = weight;
        Cost = cost;
    }
}

I can think of two simple ways to deal with this, but I am uncertain which one I should go with.

First one is to add a reaodnly field to Entity and request is via the constructor, so Entity would look like this:

abstract class Entity
{
    private readonly EntityData data;

    protected Entity(EntityData data)
    {
        this.data = data;
    }
}

While an extending class, Table for example, would look like this:

sealed class Table: Entity
{
    public Table() : base(EntityData.Table) {}
}

The second option would be to add an abstract, protected property (getter only) to Entity. In this case Entity would look like this:

abstract class Entity
{
    protected abstract EntityData Data { get; }
}

While the extending class Table would look like this:

sealed class Table : Entity
{
    protected override EntityData Data
    {
        get { return EntityData.Table; }
    }
}

Thus internally, Entity would simply use the property instead of a field.


The performance differences are probably in the realm of micro-optimization, but I would still like to take them into consideration.

The first way obviously increases the size of any Entity object.
The first way also forces all the extending types to define their constructors and call the base constructor or will simply increase the number of parameters in the already existent constructor.

The second way doesn't increase the size of objects, and (to me) look shorter and slightly more concise.
Performance-wise however, benchmarking showed that a virtual method call is much slower than accessing a field.
It is also less obvious that such a property is a dependency of Entity, something the first way makes perfectly clear.

As there's no clear-cut answer here, I would simply love to have a few opinions on the matter, and which of the two ways you find 'better'.
Thanks in advance.

(Please no responses about how this is in the realm of micro optimization and thus I shouldn't waste my time with it. I simply find this interesting.)

\$\endgroup\$
2
\$\begingroup\$

I think I personally like the set by constructor method in general. I think it provides a means by which you can better allow for TDD and also implicitly implies the contract all inheriting classes must obey in providing a Entity object if they wish to inherit from the base class.

I think if I were going make Entity a requirement of the second method I would make the protected method abstract which would also enforce this constraint. However that doesn't allow for the easy of use in using dependency injection which I think would suit this approach nicely.

I'm not sure why there would be a huge increase in the size of the Entity object itself. I would assume the size increase would only be as large as the size of the memory register holding the location of the Entity object itself. As for performance, I can't comment explicitly but I would assume you would still need to make visible the EntityData object to it's children so you would probably do that via a property accessor or method anyway so performance impacts to me would be low on the priority list unless profiling the code suggested otherwise.

So assuming the Entity is required in inherited classes I would consider something like:

abstract class Entity
{
    protected EntityData Data { get; private set; }

    protected Entity(EntityData data)
    {
        this.Data = data;
    }
}
\$\endgroup\$
2
\$\begingroup\$

While Jesse's position that fields should never be public has merit, dreza's use of auto-properties is a much easier way to accomplish almost exactly the same ends:

sealed class EntityData
{
    static EntityData()
    {
        table = new EntityData(250, 60);
        chair = new EntityData(120, 25);
    }    

    private EntityData(int cost, int weight)
    {
        this.weight = weight;
        this.cost = cost;
    }

    public static EntityData Table{get; private set;}
    public static EntityData Chair{get; private set;}

    public int Cost{get; private set;}
    public int Weight{get; private set;}
}

The one thing this doesn't save you from is Joe Programmer implementing some method within this class that mutates Cost or Weight, requiring slightly more discipline in code reviews (and Joe Programmer can do similar damage to pretty much any other implementation; the readonly keyword on a field will just show him he's wrong, and then he's double-wrong if he removes it). If you ever needed to convert this to a property with a backing field, that can be done trivially by any refactoring tool you can plug in (and if you aren't using something like ReSharper or CodeRush, you're not doing it right).

BTW, regarding public static readonly fields, I don't have much bad to say about them, and I don't think they should be avoided out of hand. C# In Depth states that using a public static readonly field for the instance member of a singleton (or here, a multiton) is not only fully thread-safe, it's reasonably lazy when used as a true singleton. In a multiton situation, the constructor's invoked, creating all static instances, on the first reference to any one of them; using the Lazy<T> structure will get you lazier and still thread-safe operation.

I would avoid exposing instance-level fields (readonly or otherwise) if you also use properties. This combination of member types makes reflectively accessing members more difficult because you not only have to scan the type for member fields, but also for member properties. Public fields have additional drawbacks vs property accessors, but if there weren't some merit to being able to expose them, then the C# team simply would have disallowed use of public on a field, n'est-ce pas?.

\$\endgroup\$
0
\$\begingroup\$

Maybe I'm a bit of a stick in the mud on this, but I simply can't let member variables be public -- no matter how readonly or static they are. So here's a slightly altered version that makes the variables private and exposes them via read-only properties. I'm also a fan of interfaced classes, particularly for unit testing, but also for dependency-injection, etc.

interface IEntityData
{
    int Cost
    {
        get;
    }

    int Weight
    {
        get;
    }
}

sealed class EntityData : IEntityData
{
    private static readonly IEntityData table = new EntityData(250, 60);
    private static readonly IEntityData chair = new EntityData(120, 25);

    private readonly int cost;
    private readonly int weight;

    private EntityData(int cost, int weight)
    {
        this.weight = weight;
        this.cost = cost;
    }

    public static IEntityData Table
    {
        get
        {
            return table;
        }
    }

    public static IEntityData Chair
    {
        get
        {
            return chair;
        }
    }

    public int Cost
    {
        get
        {
            return this.cost;
        }
    }

    public int Weight
    {
        get
        {
            return this.weight;
        }
    }
}

I would do similar with dreza's snippet:

abstract class Entity : IEntity
{
    private readonly EntityData data;

    protected EntityData Data
    {
        get
        {
            return this.data;
        }
    }

    protected Entity(EntityData data)
    {
        this.data = data;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Yikes, do you get paid by the line of code? \$\endgroup\$ – Leonid Jul 9 '12 at 3:42
  • 2
    \$\begingroup\$ No, but I do get paid for not creating technical debt. \$\endgroup\$ – Jesse C. Slicer Jul 9 '12 at 3:46
  • 2
    \$\begingroup\$ I feel solid Java experience here. \$\endgroup\$ – Andrey Agibalov Aug 7 '12 at 19:27
  • 1
    \$\begingroup\$ @loki2302: Me too. There's a much better way to approach this in C#. Give me five minutes and I'll cut his LOC in half. \$\endgroup\$ – KeithS Aug 8 '12 at 16:47

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.