views:

61

answers:

4

Hi,

I have a function which is processing rows from an excel file.In this function,I have a for loop.Now,once a row is extracted,we check for various conditions.If any condition is false,we continue with next row.Can this code be made more structured using a pattern?

 foreach (System.Data.DataRow dr in ds.Tables[0].Rows)
                {

                    Product prod = GetProduct(dr);
                    if (prod == null)
                    {
                        IndicateNotInserted(dr, "InvalidProduct");
                        continue; 
                    }



                    if (IsProductExpired())
                    {
                        IndicateNotInserted(dr, "Expired");
                        continue; 
                    }
                    ProcessRow(dr);
A: 

Could you use a switch case instead perhaps?

aaronmase
+2  A: 

You could use a chain of responsibility style pattern where a there are a series of classes each one responsible for checking one aspect of validity and reporting the type of error.

you would pass your Prod to the first class which would check the validity, if it was invalid it would report and return false. if it was valid it would return the validity check of the successor of the current instance (if it had one).

then your code could just get the Prod and pass it to the root validity checker and continue if it reported false.

This would also allow you to easily add new validators in the future, potentially being able to do it without having to recompile, depending on how you implemented the construction of the chain of validators.

something like this: (pseudo code, not tested/compiled)

public interface IValidator
{
bool IsValid(Product product);
IValidator Successor{get;set;};
}

public class AbstractValidator : IValidator
{    
    IValidator m_successor=null;
    public abstract bool IsValid(Product product);

    IValidator Successor
    {
    get 
      {
      if(m_sucessor==null)
          return new AlwaysValidSuccessor();
      else
          return m_successor;
      }
    set
       {
       m_successor=value;
       }
    }
}

public class NullValidator : AbstractValidator
{
 public bool IsValid(Product product)
 {
    if (product!=null)
        {
        return Successor.IsValid(product);
        }
    return false;
    }
}

public class ExpiredValidator : AbstractValidator
{
 public bool IsValid(Product product)
 {
    if (product.Expired==false) //whatever you need to do here
        {
        return Successor.IsValid(product);
        }
    return false;
    }
}

then you use it:

IValidator validatorRoot = new NullValidator();
validatorRoot.Successor = new ExpiredValidator();
// construct the chain with as many as you need
//then use it
if (validatorRoot.IsValid(Prod)==false)
{
     continue;
}
Sam Holder
yeh,i too was thinking in terms of chain of responsibility pattern.thanks
jess
This is also more flexiable as you can add more conditions later or remove them much more easily. Another advantace is that you are programming you validation against interfaces, so mocking and stubbing is much easier and even replacing the implmentation is easier.
the_drow
Thing is, if you have invalid data, in most cases you want to know *why* it's invalid. The validators need to return (or at least provide the ability to retrieve) something more specific than "valid" or "invalid", unless you intend to check each one -- and checking each one would negate most, if not all, of the benefits of using this pattern.
cHao
The validators could log that information (which I intimated at but didn't include in my examples) or provide that information a number of ways, to an object passed through all the validators, or via an event etc etc. The invalid information being filtered at the query level would not help with knowing which rows were invalid and why they were invalid.
Sam Holder
That's because properly filtered data won't have invalid rows. Without filtering, the function has two tasks (processing the valid data, and reporting the invalid data) that can't be separated. The validation and reporting should really be in another function entirely, and only the valid data submitted for processing.
cHao
To be clear, in the validation function, CORP (or at least a variant of it) could be a really good idea. But doing the validation and processing at the same time leads to either inflexibility or duplication of code in the cases where you might want to validate data without processing it, which would be really helpful.
cHao
+1  A: 

The best way to fix this is to define your queries such that they don't return the invalid and out-of-date products.

cHao
+1, good point, as the answer to 'How can I use a pattern?' question often ought to be rephrased 'Should I use a pattern?'
Jacob Oscarson
Whilst this may be appropriate, it may be that they need to process all data and report the invalid data, or they may be doing this for any number of perfectly valid reasons. This does not answer the question, and should IMHO be a comment to the question and not an answer.
Sam Holder
Sometimes the question asked isn't the question that needs answering.
cHao
@cHoa, that is very true, and I'm not disagreeing with you or your answer. I still think the comment vs answer thing is still valid though.
Sam Holder
A: 

In combination of Sam Holder, depeanding how generic you need this validation system to be, you can wrap any of the responsibilities with a Decorator to enable to wrap the validation check with common behavior that is joint more then one validation check.

the_drow