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.

I've recently made a set of interfaces/classes to work with converting a spreadsheet into an object but I utilise an empty interface in my design:

So first off I have my interface which defines what a spreadsheet object should have:

public interface IParsedSpreadsheet<TEntity> where TEntity: IParsedRow
{
    int Columns { get; set; }
    int Pages { get; set; }
    Dictionary<string, int> Map { get; set; }
    List<string> ColumnHeaders { get; set; }
    List<TEntity> RowList { get; set; }
    List<TEntity> ParseSheet(IFileStorage storage);
    List<String> ObtainColumnHeaders(IFileStorage storage);
}

As you can see it take a type parameter which must implement IParsedRow which is my empty interface:

public interface IParsedRow
{
    //Marker Interface
}

Below is an abstract class declaration that implements the interface:

public abstract class AbstractParsedSpreadsheet<TEntity> : IParsedSpreadsheet<TEntity>, IEnumerable<TEntity> where TEntity : IParsedRow, new()

One of the important parts of this class is the Map. This has a list of column headers that should appear in a spreadsheet and also the index of the column they appear in.

Any concrete implementation of IParsedRow will have a number of properties each of which must be named after a column header in the spreadsheet it will represent. I use the Map along with reflection to determine that the spreadsheet headers match upto the TEntity's properties exactly to let me know that the uploaded spreadsheet is valid for the map it is being put against.

Here is my abstract class in full (note I use NPOI as a wrapper to easily parse a spreadsheet, NPOI is an open source spreadsheet framework for .NET):

    /// <summary>
    /// Abstract, generic class that only accepts a type parameter which implements IParsedRow for use in its internal row collection.
    /// </summary>
    /// <typeparam name="TEntity"></typeparam>
    public abstract class AbstractParsedSpreadsheet<TEntity> : IParsedSpreadsheet<TEntity>, IEnumerable<TEntity> where TEntity : IParsedRow, new()
    {
        public int Columns { get; set; }
        public int Pages { get; set; }
        public Dictionary<string, int> Map { get; set; }
        public List<string> ColumnHeaders { get; set; }
        public List<TEntity> RowList { get; set; }

        /// <summary>
        /// Single parametised constructor which will perform all work needed to make an object from a spreadsheet.
        /// Used on the front end this might have a dropdown list of all "maps" or spreadsheet types you can upload.
        /// and then the act of uploading will create a storage item. If the spreadsheet in the storage item doesnt match
        /// up to the map then an error will be thrown.
        /// </summary>
        /// <param name="validationMap">this should hold the names of the columns in the spreadsheet (and also the TEntities properties) and the column index in which the column name resides in</param>
        /// <param name="storage">this object has a property which refers to a file location and is used by NPOI to load up a spreadsheet for checking and parsing.</param>
        public AbstractParsedSpreadsheet(Dictionary<string,int> validationMap, IFileStorage storage)
        {
            this.Map = validationMap;

            //Check validation map against properties of TEntity
            if (!this.CheckMapMatchesRowType())
            {
                throw new InvalidDataException("Invalid Map/Type parameter used");
            }

            //Obtain column headers from spreadsheet
            this.ColumnHeaders = ObtainColumnHeaders(storage);

            //Check validationMap against column headers
            if (!CheckHeadersAgainstMap())
            {
                throw new InvalidDataException("Invalid Spreadsheet/Map used");
            }

            //Parse spreadsheet into RowList if all of the above pass.
            this.RowList = ParseSheet(storage);
        }

        /// <summary>
        /// This method takes in an IFileStorage implementation and uses it to locate and open a spreadsheet.
        /// It then reads formthe spreadsheet, calling another function to create objects of type TEntity
        /// and adds them into a list which belongs to this class.
        /// </summary>
        /// <param name="storage"></param>
        /// <returns></returns>
        public List<TEntity> ParseSheet(IFileStorage storage)
        {
            List<TEntity> ListOfRows = new List<TEntity>();

            HSSFWorkbook hssfbook;

            using (FileStream file = new FileStream(storage.StorageLocation, FileMode.Open, FileAccess.Read))
            {
                hssfbook = new HSSFWorkbook(file);
            }

            ISheet sheet = hssfbook.GetSheetAt(0);

            foreach (IRow row in sheet)
            {
                if (row.RowNum == 0)
                {
                    continue;
                }
                else
                {

                    ListOfRows.Add(CreateEntityFromRow(row));
                }
            }

            return ListOfRows;
        }

        /// <summary>
        /// Bit of a complicated one - Accepts an IRow implementing object (those used by the NPOI spreadsheet classes)
        /// looks up the column index of each cell in a row and maps it using the local Map variable (dictionary of string to int)
        /// to a string value. This value can then be used to dynamically obtain a property name from TEntity using .NET Reflection.
        /// The value of the current cell is then set to that property on TEntity before being continuing to the next cell.
        /// After the entire object is populated it returns it.
        /// </summary>
        /// <param name="row"></param>
        /// <returns></returns>
        public TEntity CreateEntityFromRow(IRow row)
        {
            TEntity retVal = new TEntity();
            Type entity = typeof(TEntity);
            int propertyCount = entity.GetProperties().Count();

            foreach (ICell c in row)
            {
                //Looks up the column index of the current cell and Maps it to the corresponding value in the Map dictionary to 
                //obtain the correct property name in TEntity that this value needs to be set for.
                string columnName = this.Map.Where(d => d.Value == c.ColumnIndex).Select(e => e.Key).First();

                switch (c.CellType)
                {
                    case CellType.STRING:
                        retVal.GetType().GetProperty(columnName).SetValue(retVal, c.StringCellValue.ToString(), null);
                        break;
                    case CellType.NUMERIC:
                        retVal.GetType().GetProperty(columnName).SetValue(retVal, c.NumericCellValue, null);
                        break;
                    case CellType.BOOLEAN:
                        retVal.GetType().GetProperty(columnName).SetValue(retVal, c.BooleanCellValue, null);
                        break;
                    case CellType.BLANK:
                    case CellType.ERROR:
                    case CellType.FORMULA:
                    case CellType.Unknown:
                    default:
                        break;
                }
            }

            return retVal;

        }
        /// <summary>
        /// Looks up the generic parameter for this class, instatiates it and checks that its properties match the map.
        /// It then checks to ensure that the map contains the correct number of entries for the number of properties on
        /// the generic type.
        /// </summary>
        /// <returns></returns>
        public bool CheckMapMatchesRowType()
        {
            Type entity = typeof(TEntity);
            var properties = entity.GetProperties();

            if (properties.Count() != Map.Count)
            {
                return false;
            }

            foreach (var i in properties)
            {
                if (!Map.Keys.Contains(i.Name.ToLower())){
                    return false;
                }
            }

            return true;
        }

        /// <summary>
        /// Gets the top row of any spreadsheet (which is normally where the headers are)
        /// </summary>
        /// <param name="storage"></param>
        /// <returns></returns>
        public virtual List<string> ObtainColumnHeaders(IFileStorage storage)
        {
            HSSFWorkbook hssfbook;
            List<string> ColumnHeaders = new List<string>();

            using (FileStream file = new FileStream(storage.StorageLocation, FileMode.Open, FileAccess.Read))
            {
                hssfbook = new HSSFWorkbook(file);
            }

            ISheet sheet = hssfbook.GetSheetAt(0);
            IRow row = sheet.GetRow(0);

            foreach (ICell c in row)
            {
                switch (c.CellType)
                {
                    case CellType.STRING:
                        ColumnHeaders.Add(c.StringCellValue.ToString().Replace(" ", string.Empty));
                        break;
                    case CellType.NUMERIC:
                    case CellType.BOOLEAN:
                    case CellType.BLANK:
                    case CellType.ERROR:
                    case CellType.FORMULA:
                    case CellType.Unknown:
                    default:
                        break;
                }

            }

            return ColumnHeaders;
        }

        /// <summary>
        /// Checks that the headers obtained from the spreadsheet passed in are valid against the map that has been passed in
        /// also checks that the count of both of them matches.
        /// 
        /// </summary>
        /// <returns></returns>
        public virtual bool CheckHeadersAgainstMap(){
            if (ColumnHeaders.Count != this.Map.Values.Count)
            {
                return false;
            }
            foreach (string i in this.ColumnHeaders)
            {
                if (!this.Map.Keys.Contains(i.ToLower()))
                {
                    return false;
                }
            }
            return true;
        }

        /// <summary>
        /// Make the RowList propert of the class it's enumerable.
        /// </summary>
        /// <returns></returns>
        public IEnumerator<TEntity> GetEnumerator()
        {
            foreach (TEntity t in this.RowList)
            {
                if (t == null)
                {
                    break;
                }

                yield return t;
            }
        }

        [ExcludeFromCodeCoverage]
        IEnumerator IEnumerable.GetEnumerator()
        {
            return this.GetEnumerator();
        }
    }

How can I improve my design? Is my use of an empty interface in this case a code smell? Should I just remove it and allow to be of any type as the mapping functions will catch it anyway and throw an exception?

share|improve this question
1  
Why don't you use reflection on TEntity to deduce the validationMap instead of receiving it on construction, and failing on any discrepency? –  Uri Agassi Mar 3 at 13:30
    
That... Is a pretty damn good idea! EDIT: Not too sure it will work now. The map is passed in to determine the column index, as I don't want this being hard coded in the order of the properties of TEntity (I want to be able to pass it in as a parsed text file from which the map will be created) I can't see how I can make it work. –  Rondles Mar 3 at 13:31

2 Answers 2

up vote 8 down vote accepted

An empty interface is a code smell. In C# you can use attributes to mark a class instead of making it implement an empty interface.

As I suggested in my comment, you can use reflection to build your Map instead of receiving it as input, and failing on discrepancy. With the power of attributes, you can add functionality by marking which properties you want to map, which to ignore, maybe add metadata (like the column index):

[AttributeUsage(AttributeTargets.Class)]
public class ParsedRowAttribute : Attribute {
}

[AttributeUsage(AttributeTargets.Property)]
public class ParsedCellAttribute : Attribute {
    public ParsedCellAttribute(int columnIndex) {
        this.ColumnIndex = columnIndex;
    }

    public int ColumnIndex { get; set; }
}

If you want to add the column index from an outside source, you can use it as overrides to the code, so you can still count on the class to tell you which properties it externalizes, and validate the override mapping to see there are no typos there.

share|improve this answer
    
Thanks. So can you specify that a particular Type Parameter must be decorated with a specific attribute (e.g. where TEntity : ParsedRowAttribute) –  Rondles Mar 3 at 13:48
    
@Rondles No, you can't. If you wanted to ensure that, you'd have to do a check somewhere- probably the constructor. –  Ben Aaronson Mar 3 at 13:51
    
Right I see. Well after all of this I have realised that my interface could actually do with at least one property which is row number which I've realised i'll need for anything which is a Row. So I guess that eliminates the code smell. Although attributes have intrigued me now as I think other parts of my code can benefit. –  Rondles Mar 3 at 13:53
    
@Rondles That sounds sensible, though in general you should make sure your mentality is "I need this property so an interface is the right tool for the job" rather than "I want an interface, so what would be a nice property to put on it?" –  Ben Aaronson Mar 3 at 13:56
1  
@Rondles While compile time errors are preferable to run time, I'm not sure that really falls under the realm of code smells. Having said that, this isn't exactly the normal use for an attribute. Perhaps you should think about whether any specific marking in code of a Parsed Row is really needed at all. In my experience, classes like these (e.g. Model classes in ASP.NET MVC) don't have any special marking. Conceptually all you're really saying is that it has publically accessible properties –  Ben Aaronson Mar 3 at 14:10

It may appear to some as code smell, but it's a practice in use in the .net framework defined as 'marker interfaces'.

IReadOnlySessionState is one of these and as per the documentation:

Specifies that the target HTTP handler requires only read access to session-state values. This is a marker interface and has no methods.

So, to answer your question, is this a code smell? perhaps it depends who you ask. In my opinion, if it's good enough 'pattern' for the .net framework, it's good enough for me.

http://msdn.microsoft.com/en-us/library/system.web.sessionstate.ireadonlysessionstate(v=vs.110).aspx

share|improve this answer
1  
I too noticed the framework using some marker interfaces which still doesn't make them not a code smell =p, but at the same time they do enable polymorphism and constraints better than attributes do! –  Rondles Mar 3 at 14:44
    
I also think how you are using the interface indicates if it is a code smell or not. For instance if I have 2 empty interfaces that implement a common generic interface for the soul purpose of differentiating the implemented classes for dependency injection. –  Jeff Vanzella Mar 3 at 17:46

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.