Validating Excel file columns

Question

I'm looking for ways to improve this code (more readable, less redundant and maybe cleaner/faster).

The problem I needed to solve:

I was designated to implement a software that is going to validate an 96 column Excel file and if there is no error on it, create an XML file from it. In case there are any error in the excel file I have to display them to the user and indicate where the problem occurred.

How I attempted to solve the problem:

Since I knew the numbers of columns, I thought about making a class that represents an cell on the Excel file and include a string property to hold an possibly error description, by making that it'd make it easy to display the error log. So I actually created 2 collections of my class that is called Cell. One collection to hold all of Excel's cell values, and the other is the error log one.

Here is the Cell class code:

public class Cell
{
    public string Value { get; set; }
    public int Row { get; set; }
    public int Column { get; set; }
    public string ErrorDescription { get; set; }
}

This is the Cells collection:

Range worksheetCells = sheet.get_Range(firstCell, lastCell);
private List<Cell> Cells = new List<Cell>();

foreach (Range item in worksheetCells)
{
     Cells.Add(new Cell{ Value = item.Text, Row = item.Row, Column = item.Column});             
}

For readability purposes, I create another 96 collections of Cell, one for each column:

var Name = Cells.Where(c => c.Column == 1);
....

I've created 96 methods of validation one per column/collection:

private void NameValidation(IEnumerable<Cell> excelColumn, List<Cell> log)
{
    foreach (Cell item in excelColumn)
    {
        if (string.IsNullOrEmpty(item.Value))
        {
            item.ErrorDescription = "You need to fill up this Cell.";
        }
        else
        {
            if (item.Value.Length > 27)
            {
                item.ErrorDescription = "The MAX length of this field is 27 characters.";
            }
        }

        if (!string.IsNullOrEmpty(item.ErrorDescription))
        {
            log.Add(item);
        }
    }
}

Basically that is what I have now. I haven't touch the XML file creation yet, but the reason why I've designed my code like this is that I can use the validation method to create the XML, instead of having to do another 96 foreach statements. I'd only create the XML file when after calling all the validation methods if the log.count return 0, so it is ok to generate the file.

What troubles my mind:

Do I really need to do 96 things all the time?

in 60% of the the validation methods I have to check for string.IsNullOrEmpty(item.Value) and copy and paste kills me, It makes me feel like I'm doing something wrong, you know?

In a few of those methods, the item.value is only mandatory if another cell is filled up. If it is, then the current cell is mandatory. That's why I didn't make a single method to validate when the field is mandatory, because it depends on other things. In 30% of the validation methods, I have to check for item.Value.Length > "something".

Do you have any ideas on how to take some of those if/else out?

Answer 1

If you find yourself repeating methods that often, then yes, you need to find a pattern that simplifies your work. That said, I think there would be a better solution than loading up collections of cells in columns.

Take a look at this sample: validate excel sheet

Granted, the sample code at the other end of the link is validating an excel sheet prior to inserting into an Sql data store. But the gist is the same.

I would query and hold in a DataTable. Properties can be set on each column that allow much easier validation of the data. In addition, you do not have to write code to manage the collections.

Answer 2

Consider using custom attributes.
Here is a possible variant:
Create a class representing the data from the excel file and decorate it with attributes.

    public class ExcelData
    {
        [ExcelColumn("Name")]
        [Required]
        [MaxLength(27)]
        public string Name { get; set; }

        [ExcelColumn("NextColumn")]
        [Required]
        public string NextColumn { get; set; }
        //...another 94 property.
    }        

Then create a class that gets excel and returns a list of this data using ExcelColumn attribute.

    public class ExcelParser
    {
        public IEnumerable<ExcelData> Parse(string fileName)
        {
            //Get file and return ExcelData, using ExcelColumn attribute.
        }
    }

Then create a class to convert this data to XML.

public class ExcelDataToXmlConverter
    {
        private ExcelDataValidator _validator = new ExcelDataValidator();

        public XDocument Convert(IEnumerable<ExcelData> excelData)
        {
            foreach (var data in excelData)
            {
                var errors = new Dictionary<string, List<string>>();
                if (_validator.IsValid(data, out errors))
                {
                    //Convert the row to XML
                }
                else
                {
                    //Do something with errors.
                }
            }
        }            
    }

    public class ExcelDataValidator
    {
        //errors - is a list of errors where the key is the name of the field and value is the list of errors.
        public bool IsValid(ExcelData excelData, out Dictionary<string, List<string>> errors)
        {
            //Validate using different attributes: Required, MaxLength and so on.
        }
    }

Maybe you'll be able to use attributes for actual converting data to XML.
Of course it's only a very crude sketch, and only some parts of it may be useful for you, but I hope you've got the idea.

Answer 3

Edit: you may want the funcs to return a string rather than a bool as you have stated you want descriptive validation information, then instead of just iffing for boolean true, you would make the if in the AllCellsAreValid, you would if(!string.IsNullOrEmpty(columnValidatorMap[cellToValidate.Column](cellToValidate)))

Edit again: also, you're creating 96 lists you said I think? just create one list with every single cell regardless of column, and when you hand that list to the validator that checks all cells, it will just use the column member of Cell to find the correct validator in the dictionary.

Try this, create a validator method for the various types of validation you will be doing and map them to the columns like so:

    public static Func<Cell, bool> LengthValidator(int minLength, int maxLength)
    {
      return (Func<Cell, bool>)((cellParam) => { return (cellParam.Value.Length > minLength) && (cellParam.Value.Length < maxLength); });
    }

    public static Func<Cell, bool> NullnessValidator(bool shouldBeNull)
    {
      return (Func<Cell, bool>)((cellParam) => { return ((cellParam == null) == shouldBeNull); });
    }

    public static Func<Cell, bool> MultipleValidators(params Func<Cell, bool>[] validatorPredicates)
    {
      return (Func<Cell, bool>)((cellParam) =>
        {
          foreach(Func<Cell,bool> validator in validatorPredicates)
          {
            if(!validator(cellParam))
            {
              return false;
            }
          }
          return true;
        });
    }

    public static Dictionary<int, Func<Cell, bool>> columnValidatorMap = new Dictionary<int, Func<Cell, bool>>()
      {
        { 1, MultipleValidators(LengthValidator(1, 27), NullnessValidator(false)) },
        { 2, MultipleValidators(LengthValidator(1, 27), NullnessValidator(false)) }
      };

    public bool AllCellsAreValid(IEnumerable<Cell> cellsToValidate)
    {
      foreach(Cell cellToValidate in cellsToValidate)
      {
        if (!columnValidatorMap[cellToValidate.Column](cellToValidate))
        {
          return false; // or return some information if you want
        }
      }

      return true;
    }