2
\$\begingroup\$

I still feel like I struggle to grasp some concepts of SOLID principles at times, and am hoping for some clarification here.

The following classes are entities in an Entity Framework project.

namespace MyCompany.MyProduct.Core {
    public abstract class DataError {
        public int ErrorId { get; set; }

        public int DataItemId { get; set; }

        public DataItem Data { get; set; }
    }

    public class NoMediaError : DataError {
    }

    public class TooManyMediaItemsError : DataError {
        public ICollection<MediaItem> MediaItems { get; set; }
    }

    public class MediaItemConversionError : DataError {
        public string Description { get; set; }
        public MediaItem Item { get; set; }
    }
}

The above entities live in a project called MyCompany.MyProduct.Core.

The solution itself contains other projects, one of which is an MVC website (MyCompany.MyProduct.Web), and the other is a WCF service (MyCompany.MyProduct.Workflow). Both of these projects reference the MyCompany.MyProduct.Core project, as both use the Entity Framework project, but they don't reference each other.

In the MVC project, I have a page that displays a summary of a DataError. The summary, and some of the page contents depends on the concrete type of the DataError. For example, at the minimum, the page should display a sentence that describes the error. This could be a static string, like "No Media Items were found for this data item", or a more dynamic one such as "5 media items were found for this data item.". How to implement this is what I'm struggling with.

In my mind, the simplest way to implement it is to add an abstract method to the DataError class, like this:

public interface IDataErrorViewHelper() {
    HtmlString GetErrorDescription();
    HtmlString GetSuggestedResolutions();
}

public abstract class DataError {
    public int ErrorId { get; set; }
    public int DataItemId { get; set; }
    public DataItem Data { get; set; }
    public abstract IUnlinkableViewHelper GetViewHelper();
}

I like this because it means that each subtype of DataError can return its own implementation of IDataErrorViewHelper. My main issue with it however is that in my mind, the responsibility of the DataError class is to store the error state, and NOT to provide MVC view helpers. Additionally, I would have to add a reference to the MVC assemblies in order to use HtmlString, and given that this MyCompany.MyProduct.Core project isn't even aware that there exists a MyCompany.MyProduct.Web project, it feels wrong to add the IDataErrorViewHelper into it - like it is mixing the responsibilities of the assemblies/projects - I have to add a reference to System.Web solely to reference HtmlString!

My other idea was to create a DataErrorType enum:

public enum DataErrorType {
    NoMedia,
    TooManyMediaItems,
    MediaItemConversion
}

public abstract class DataError {
public int ErrorId { get; set; }
public int DataItemId { get; set; }
public DataItem Data { get; set; }
public abstract DataErrorType Type { get; }
}

This would allow me to create the following extension method in my MyCompany.MyProduct.Web project:

namespace MyCompany.MyProject.Web {
    public interface IDataErrorViewHelper() {
        HtmlString GetErrorDescription();
        HtmlString GetSuggestedResolutions();
    }

    public static class DataErrorExtensions {
        public static IDataErrorViewHelper GetViewHelper(this DataError error) {
            switch (error.Type) {
                case DataErrorType.NoMedia: return new NoMediaViewHelper((NoMediaError)error);
                case DataErrorType.TooManyMediaItems: return new TooManyMediaItemsViewHelper((TooManyMediaItemsError)error);
                case DataErrorType.MediaItemConversion: return new ConversionErrorViewHelper((MediaItemConversionError)error);
                default: throw new NotSupportedException("The error type is not supported.");
            }
        }
    }
}

I like this approach because it allows all MVC/Web code to remain in the MyCompany.MyProduct.Web project instead of infecting the MyCompany.MyProduct.Core one. However I'm not keen on the fact that I have to add an Enum to the DataError class just to facilitate this, especially when the Enum is just a shortcut to determining the type of the DataError. Also, I don't like that there's a switch statement to create the correct IDataErrorViewHelper - it seems like having it created from the abstract type would be the perfect way to implement it, but that's what I did in the previous approach and I had issues with that way too...

Does anyone have any better ways to implement this? If not, which of the ways that I've described do you recommend?

\$\endgroup\$
  • \$\begingroup\$ For me this is looking more like a question for programmers. Check their Help center \$\endgroup\$ – Heslacher Jan 22 '15 at 6:26
  • \$\begingroup\$ Agreed, while it is concrete code and therefore not off-topic, there isn't really that much for us to critique, aside from architecture. \$\endgroup\$ – Nick Udell Jan 22 '15 at 11:16
  • \$\begingroup\$ @NickUdell Thats why I didn't voted to close and didn't said it was off topic. \$\endgroup\$ – Heslacher Jan 22 '15 at 11:33
  • \$\begingroup\$ Indeed, and that's why I said I agreed. I was just expanding for the OP's information. \$\endgroup\$ – Nick Udell Jan 22 '15 at 11:39
2
\$\begingroup\$

Properties

I'm not sure why you have public setters for your properties in DataError. It doesn't make sense to me to provide the ability to change what the error id was after the fact, for example. Instead it would probably be better to remove the setters and rely on the constructor to fill the values, since those values are really only variable at construction.

public abstract class DataError 
{
    public int ErrorId { get; private set; }

    public int DataItemId { get; private set; }

    public DataItem Data { get; private set; }

    protected DataError(int errorId, int dataItemId, DataItem Data)
    {
        this.ErrorId = errorId;
        this.DataItemId = dataItemId;
        this.Data = data;
    }
}
\$\endgroup\$

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.