Code review: Fabric class

June 1, 2017

This is the second code review session where I showcase a real-world code example. If you want to learn more about this new format, check out this post. You can also request a review yourself. To do that, use the form on this page.

Code review: Fabric class

The full source code of this one is rather large for a single blog post, so I decided to pick the most interesting portions of it - those I think could be improved.

Even after the reduction, it remains to be too large to inline here in full. So instead, I will provide excerpts while discussing different parts of it. But you can go ahead and see the full version on GitHub.Gist.

The code sample belongs to an e-commerce application for a clothes business. The part of the domain model we’ll be focusing on consists of Fabric, FabricImage, and FabricTag classes. There’s a many-to-many relationship between Fabric and FabricImage, as well as between Fabric and FabricTag. Fabric also contains pre-order information: PreOrder and Due fields.

Here’s the part of the class we’ll start with:

public class Fabric : AggregateRootWithLinkedObjects
{
    public bool PreOrder { get; protected set; }

    private string _Due { get; set; }
    public Maybe<string> Due
    {
        get => _Due;
        protected set => _Due = value.Unwrap();
    }

    protected virtual ICollection<FabricImage> _Images { get; set; }
    public IEnumerable<FabricImage> Images => _Images.Skip(0);

    protected virtual ICollection<FabricTag> _Tags { get; set; }
    public IEnumerable<FabricTag> Tags => _Tags.Skip(0);

    internal static Fabric Create(
        int[] imageIds, int[] tagIds, bool preOrder, Maybe<string> due)
    {
        Guard.ForDuplicates(imageIds, "Images");
        Guard.ForDuplicates(tagIds, "Tags");

        var fabric = new Fabric();

        if (preOrder)
        {
            if (due.HasNoValue)
            {
                throw new InvalidOperationException(
                    "A pre ordered fabric must have due set");
            }
            fabric.SetPreOrder(due.Value);
        }
        else
        {
            if (due.HasValue)
            {
                throw new InvalidOperationException(
                    "A pre ordered fabric does not need a due set");
            }
        }

        fabric._Images = imageIds
            .Select(id => new FabricImage(fabric.Id, id))
            .ToList();

        fabric.MakeFirstImageDefault();

        fabric._Tags = tagIds
            .Select(id => new FabricTag(fabric.Id, id))
            .ToList();

        return fabric;
    }

    private void SetPreOrder(string due)
    {
        PreOrder = true;
        Due = due;
    }

    private void HasArrived()
    {
        PreOrder = false;
        Due = null;
    }

    internal void Update(Fabric update)
    {
        if (update.PreOrder)
        {
            SetPreOrder(update.Due.Value);
        }
        else
        {
            HasArrived();
        }

        UpdateLinkedObjects(_Tags, update.Tags, AddFabricTag);
        UpdateLinkedObjects(_Images, update.Images, AddFabricImage);
        UpdateDefaultImage(update);
    }
}

First off, I want to start with the things I like. The use of Maybe is one of them. This public property here:

private string _Due { get; set; }
public Maybe<string> Due
{
    get => _Due;
    protected set => _Due = value.Unwrap();
}

explicitly tells the reader that the due information might not exist in a fabric. To make this work with Entity Framework, the author applies a small trick. It is not the public property itself which is mapped to the database (such mapping would fail as EF knows nothing about Maybe) but the underlying string field. The public property here acts as a proxy which converts that string to and from a Maybe. This technique brings honesty to the class’s public API which is one of the key principles of functional programming.

I also like the guard clauses in the Create static method:

internal static Fabric Create(
    int[] imageIds, int[] tagIds, bool preOrder, Maybe<string> due)
{
    Guard.ForDuplicates(imageIds, "Images");
    Guard.ForDuplicates(tagIds, "Tags");

    /* ... */
}

They explicitly denote the class’s preconditions, and it’s a good idea to do that even if you don’t practice Design by Contract much.

Alright, let’s move on to the "could be improved" part.

First of all, note that the fields PreOrder and Due are coupled to each other. They tend to change together:

private void SetPreOrder(string due)
{
    PreOrder = true;
    Due = due;
}

private void HasArrived()
{
    PreOrder = false;
    Due = null;
}

and their preconditions depend on each other’s state:

internal static Fabric Create(
    int[] imageIds, int[] tagIds, bool preOrder, Maybe<string> due)
{
    /* ... */

    if (preOrder)
    {
        if (due.HasNoValue)
        {
            throw new InvalidOperationException(
                "A pre ordered fabric must have due set");
        }
        fabric.SetPreOrder(due.Value);
    }
    else
    {
        if (due.HasValue)
        {
            throw new InvalidOperationException(
                "A pre ordered fabric does not need a due set");
        }
    }

    /* ... */
}

This coupling is a strong sign that we are dealing with a hidden abstraction, and it’s a good idea to introduce a separate class for it. This abstraction doesn’t have its own identity, and so it can be represented as a Value Object. Here’s how it can look like:

public class PreOrderInfo : ValueObject<PreOrderInfo>
{
    public bool PreOrder { get; }

    private readonly string _due;
    public Maybe<string> Due => _due;

    public PreOrderInfo(Maybe<string> due, bool preOrder)
    {
        if (preOrder && due.HasNoValue)
            throw new InvalidOperationException("A pre ordered fabric must have due set");
        if (!preOrder && due.HasValue)
            throw new InvalidOperationException("A pre ordered fabric does not need a due set");

        _due = due.Unwrap();
        PreOrder = preOrder;
    }

    protected override bool EqualsCore(PreOrderInfo other)
    {
        return PreOrder == other.PreOrder && _due == other.Due;
    }

    protected override int GetHashCodeCore()
    {
        return (_due ?? "").GetHashCode() ^ PreOrder.GetHashCode();
    }

    public static PreOrderInfo PreOrdered(string due)
    {
        return new PreOrderInfo(due, true);
    }

    public static PreOrderInfo Arrived()
    {
        return new PreOrderInfo(null, false);
    }
}

I’m not a fan of the name (because of the "Info" suffix) but I’m not familiar with this domain, so can’t think of anything better.

Here’s how Fabric looks like after the extraction:

public class Fabric : AggregateRootWithLinkedObjects
{
    public virtual PreOrderInfo PreOrderInfo { get; set; }

    protected virtual ICollection<FabricImage> _Images { get; set; }
    public IEnumerable<FabricImage> Images => _Images.Skip(0);

    protected virtual ICollection<FabricTag> _Tags { get; set; }
    public IEnumerable<FabricTag> Tags => _Tags.Skip(0);

    protected Fabric()
    {
        _Images = new Collection<FabricImage>();
        _Tags = new Collection<FabricTag>();
    }

    internal static Fabric Create(
        int[] imageIds, int[] tagIds, bool preOrder, Maybe<string> due)
    {
        Guard.ForDuplicates(imageIds, "Images");
        Guard.ForDuplicates(tagIds, "Tags");
            
        var fabric = new Fabric();
        fabric.PreOrderInfo = new PreOrderInfo(due, preOrder);

        fabric._Images = imageIds
            .Select(id => new FabricImage(fabric.Id, id))
            .ToList();

        fabric.MakeFirstImageDefault();

        fabric._Tags = tagIds
            .Select(id => new FabricTag(fabric.Id, id))
            .ToList();

        return fabric;
    }

    internal void Update(Fabric update)
    {
        PreOrderInfo = update.PreOrderInfo;

        UpdateLinkedObjects(_Tags, update.Tags, AddFabricTag);
        UpdateLinkedObjects(_Images, update.Images, AddFabricImage);
        UpdateDefaultImage(update);
    }
}

Note several things here. First, the two properties the class was dealing with previously are now substituted with a single one: PreOrderInfo. And as the new class takes care of its own invariants, we don’t need to verify them in the Create method anymore. All we need to do is use the constructor, the required checks reside inside of it:

internal static Fabric Create(
    int[] imageIds, int[] tagIds, bool preOrder, Maybe<string> due)
{
    /* ... */

    fabric.PreOrderInfo = new PreOrderInfo(due, preOrder);

    /* ... */
}

We also don’t need to take special care of the two fields while updating the entity. Instead of this:

internal void Update(Fabric update)
{
    if (update.PreOrder)
    {
        SetPreOrder(update.Due.Value);
    }
    else
    {
        HasArrived();
    }
    /* ... */
}

private void SetPreOrder(string due)
{
    PreOrder = true;
    Due = due;
}

private void HasArrived()
{
    PreOrder = false;
    Due = null;
}

we are able to do just this:

internal void Update(Fabric update)
{
    PreOrderInfo = update.PreOrderInfo; 
    /* ... */
}

And I’ve removed the SetPreOrder and HasArrived methods. If we ever need their functionality in the future, we can use these two helper methods in PreOrderInfo itself:

public static PreOrderInfo PreOrdered(string due)
{
    return new PreOrderInfo(due, true);
}

public static PreOrderInfo Arrived()
{
    return new PreOrderInfo(null, false);
}

Overall, introducing new Value Objects helps simplify host Entities. They have fewer things to worry about and thus become much easier to maintain. The Value Objects themselves are also easier to deal with because of their immutability.

Note that you might have issues with this approach if you use Entity Framework. Its support of Value Objects is limited and it’s not always possible to marry the two. However, this particular Value Object should work (didn’t test it, though). Also, note that NHibernate doesn’t have such shortcomings.

Alright, let’s look at the part of Fabric that deals with image and tag collections:

public class Fabric : AggregateRootWithLinkedObjects
{
    protected virtual ICollection<FabricImage> _Images { get; set; }
    public IEnumerable<FabricImage> Images => _Images.Skip(0);

    protected virtual ICollection<FabricTag> _Tags { get; set; }
    public IEnumerable<FabricTag> Tags => _Tags.Skip(0);

    private void MakeFirstImageDefault()
    {
        FabricImage firstImage = _Images.FirstOrDefault();
        if (firstImage != null)
        {
            firstImage.SetIsDefault(true);
        }
    }

    internal void Update(Fabric update)
    {
        /* ... */
        UpdateDefaultImage(update);
    }
        
    private void UpdateDefaultImage(Fabric update)
    {
        int? newDefaultImageId = update.GetDefaultImageId();
        if (newDefaultImageId == null)
            return;
            
        Maybe<FabricImage> oldDefaultImageOrNothing = GetDefaultImage();
        if (oldDefaultImageOrNothing.HasValue)
        {
            var oldDefaultImage = oldDefaultImageOrNothing.Value;
            if (oldDefaultImage.ImageId == newDefaultImageId)
                return;

            oldDefaultImage.SetIsDefault(false);
        }
        var newDefaultImage = _Images.SingleOrDefault(
            i => i.ImageId == newDefaultImageId);
        newDefaultImage.SetIsDefault(true);
    }

    public Maybe<FabricImage> GetDefaultImage()
    {
        return _Images.SingleOrDefault(i => i.IsDefault);
    }

    internal int? GetDefaultImageId()
    {
        if (_Images.Count > 0)
        {
            return GetDefaultImage().Value.ImageId;
        }
        return null;
    }
}

First of all, when defining a public member, it is better to use IReadOnlyList as a return collection type instead of IEnumerable. Use IEnumerable only when you specifically want to enable lazy evaluation. I wrote more about it here: IEnumerable vs IReadOnlyList. And there’s also no need in calling Skip(0). You can just use ToList, it will create a copy of the underlying collection as well.

So these two properties:

protected virtual ICollection<FabricImage> _Images { get; set; }
public IEnumerable<FabricImage> Images => _Images.Skip(0);

protected virtual ICollection<FabricTag> _Tags { get; set; }
public IEnumerable<FabricTag> Tags => _Tags.Skip(0);

Can be refactored into this:

protected virtual ICollection<FabricImage> _Images { get; set; }
public IReadOnlyList<FabricImage> Images => _Images.ToList();

protected virtual ICollection<FabricTag> _Tags { get; set; }
public IReadOnlyList<FabricTag> Tags => _Tags.ToList();

Not a big deal, but something that we can change quickly, so why not. The more worrying piece here is the use of protected properties. Ideally, you want the backing mutable collection to be private. Otherwise, there’s a possibility for inheriting classes to mess up with it. Unfortunately, this is the best we can get when using EF, it’s another shortcoming of this ORM. Here I wrote more about it: EF vs NH (case #3). With NHibernate, it would be just that:

private ICollection<FabricImage> _images;
public IReadOnlyList<FabricImage> Images => _images.ToList();

private ICollection<FabricTag> _tags;
public IReadOnlyList<FabricTag> Tags => _tags.ToList();

Next, there’s a room for improvement in how the class handles the default image. Instead of having an IsDefault flag in FabricImage, you could introduce a new DefaultImage property in Fabric itself. Changing the default image, then, would be reduced to just updating that property. Here’s how the whole class will look like after the refactoring:

public class Fabric : AggregateRootWithLinkedObjects
{
    public virtual PreOrderInfo PreOrderInfo { get; set; }

    protected virtual FabricImage _DefaultImage { get; set; }
    public Maybe<FabricImage> DefaultImage => _DefaultImage;

    protected virtual ICollection<FabricImage> _Images { get; set; }
    public IReadOnlyList<FabricImage> Images => _Images.ToList();

    protected virtual ICollection<FabricTag> _Tags { get; set; }
    public IReadOnlyList<FabricTag> Tags => _Tags.ToList();

    protected Fabric()
    {
        _Images = new Collection<FabricImage>();
        _Tags = new Collection<FabricTag>();
    }

    internal static Fabric Create(
        int[] imageIds, int[] tagIds, bool preOrder, Maybe<string> due)
    {
        Guard.ForDuplicates(imageIds, "Images");
        Guard.ForDuplicates(tagIds, "Tags");

        var fabric = new Fabric();
        fabric.PreOrderInfo = new PreOrderInfo(due, preOrder);
            
        fabric._Images = imageIds
            .Select(id => new FabricImage(fabric.Id, id))
            .ToList();

        fabric._DefaultImage = fabric._Images.FirstOrDefault();
        // was: fabric.MakeFirstImageDefault();

        fabric._Tags = tagIds
            .Select(id => new FabricTag(fabric.Id, id))
            .ToList();

        return fabric;
    }

    internal void Update(Fabric update)
    {
        PreOrderInfo = update.PreOrderInfo;

        UpdateLinkedObjects(_Tags, update.Tags, AddFabricTag);
        UpdateLinkedObjects(_Images, update.Images, AddFabricImage);

        _DefaultImage = update._DefaultImage;
        // was: UpdateDefaultImage(update);
    }

    private void AddFabricImage(int fabricId, int imageId)
    {
        var fabricImage = new FabricImage(fabricId, imageId);
        _Images.Add(fabricImage);
    }

    private void AddFabricTag(int fabricId, int tagId)
    {
        var fabricTag = new FabricTag(fabricId, tagId);
        _Tags.Add(fabricTag);
    }
}

Note that I’ve removed all methods related to the work with default images: UpdateDefaultImage, GetDefaultImage, and GetDefaultImageId. They are not required anymore after we introduced the DefaultImage property. Also, note that this refactoring would require you to change the database structure too.

Last thing could be improved is how the class updates the linked objects - tags and images. Here’s the code again:

internal void Update(Fabric update)
{
    /* ... */
    UpdateLinkedObjects(_Tags, update.Tags, AddFabricTag);
    UpdateLinkedObjects(_Images, update.Images, AddFabricImage);
    /* ... */
}

private void AddFabricImage(int fabricId, int imageId)
{
    var fabricImage = new FabricImage(fabricId, imageId);
    _Images.Add(fabricImage);
}

private void AddFabricTag(int fabricId, int tagId)
{
    var fabricTag = new FabricTag(fabricId, tagId);
    _Tags.Add(fabricTag);
}

The UpdateLinkedObjects method is from the base AggregateRootWithLinkedObjects class which you can find here: initial version. The problem is that the aggregate root works directly with persistence concerns. Specifically, it marks objects as deleted or unchanged so that EF has an idea what to do about them:

private void MarkLinkedObjectsForDeletion<T>(
    IEnumerable<ILinkToAggregate<T>> linkedObjectsToDelete)
{
    foreach (ILinkToAggregate<T> linkedObject in linkedObjectsToDelete)
    {
        linkedObject.State = ObjectState.Deleted;
    }
}

private void MarkLinkedObjectsForKeeping<T>(
    IEnumerable<ILinkToAggregate<T>> linkedObjectsToKeep)
{
    foreach (ILinkToAggregate<T> linkedObject in linkedObjectsToKeep)
    {
        linkedObject.State = ObjectState.Unchanged;
    }
}

By default, EF treats any object you set to a property of a newly created object as also being new and tries to insert it into the database. This, of course, fails if that object already exists, and therefore you need to manually guide EF on which object it needs to insert and which not. This entails a bigger problem - coupling domain classes to concerns that have nothing to do with the domain. Unfortunately, just as with issues I mentioned previously, there’s no way around this problem if you use EF. So the only way to actually improve this part is by switching to NHibernate.

I think I saw a discussion on EF’s GitHub page where they were talking about changing this behavior, but can’t find it now and so don’t know its current status. In any case, this does seem like a small enhancement implementation wise, and NHibernate had it from the very beginning. It’s just a simple convention: if object’s Id is not the default value (zero in most cases), then attach that object to the context and not try to insert it. Only insert the object if its Id is zero. Not sure about possible breaking changes this feature might introduce, though.

Conclusion

Highlights from this code review:

Reveal hidden abstractions and introduce Value Objects for them.
Use IReadOnlyList as a return collection type in place of IEnumerable in public methods.
Don’t rely on IsDefault flag if you want to differentiate an object in a collection.

If you want me to review your own code, send it over using the submit form on this page.