14
\$\begingroup\$

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?

\$\endgroup\$
  • 2
    \$\begingroup\$ Your presented code is fine, the parts that you say are a problem are in code you didn't present, I'm having a hard time understanding what extra code you made that you don't like as it is not displayed.. \$\endgroup\$ – Jimmy Hoffa Aug 13 '11 at 20:38
  • \$\begingroup\$ See that var Name = Cells.Where(c => c.Column == 1) ? there are other 95 collections like this one, I'm not sure whether separating my code using these collection is good design or not, also there's other 95 methods like NameValidation, one for each column in the file, and sometimes they share common validations sometimes not. I don't know how to reduce the redundancy. Is it clearer? \$\endgroup\$ – Bsarkis Aug 13 '11 at 21:17
  • 2
    \$\begingroup\$ I see, this is the perfect type of thing to raise on codereview. Though if you want to show where you're being repetitive in the future, show at least 2 of whatever is repeating so it's clear what repetitions you want abstracted out. :) \$\endgroup\$ – Jimmy Hoffa Aug 13 '11 at 22:17
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'll try that and see how that goes, thanks already. \$\endgroup\$ – Bsarkis Aug 13 '11 at 21:21
5
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ Your hearts in the right place on coming up with validation patterns, but attributes cause reflection overhead that isn't necesary in this case \$\endgroup\$ – Jimmy Hoffa Aug 13 '11 at 21:36
  • \$\begingroup\$ I like the custom attribute idea but how would I deal with property that depends on other property? like there are cases in which I need to verify if a certain column has value, if it has, than the current field I'm validating is required otherwise it is not. I also like the XML idea, I'll take a look at that too, thanks for answering. \$\endgroup\$ – Bsarkis Aug 14 '11 at 2:00
  • \$\begingroup\$ @Bsarkis: As for the dependent properties validation. For example you can do the folowing: Create an attribute CustomValidation and decorate with it a property that depends on another property. Then in the method ExcelDataValidator.IsValid just check this attribute. If it is on a property then use any custom code you like to validate it. And because of IsValid gets the whole row as a parameter, you can compare the value of this property to any other property in the row. \$\endgroup\$ – Ivan Aug 14 '11 at 6:54
  • \$\begingroup\$ @Jimmy Hoffa: I know about the reflection overhead. But how much will it influence the perfomance in this particular case? Do you know? I've seen a couple of third-party libraries which use attributes to parse CSV files. I've implemented a couple myself. And I've never experienced performance problems with this approach. \$\endgroup\$ – Ivan Aug 14 '11 at 7:08
  • 1
    \$\begingroup\$ @Jimmy, totally disagree with the concern over performance. If reflection overhead is a problem, then obviously you should cache the results somewhere. It is always possible to make an attribute-based solution perform efficiently, though I agree the naive implementation that always uses Attribute.GetAttribute can be slow in certain situations. (In the worst case, an attribute-based solution should only be slow on the very first execution of the code -- types don't change and neither do their attributes, thus static caching will solve all the performance problems.) \$\endgroup\$ – Kirk Woll Aug 31 '11 at 2:44
2
\$\begingroup\$

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;
    }
\$\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.