Design pattern for implementing multiple data sources

Question

I've written a program to populate a particular object from multiple data sources, however I'm not convinced I'm going about this in the right way:

1. I have no idea which (if any) design pattern I have used (possibly abstract factory)!
2. I really don't like the implementation as it doesn't feel right.

I appreciate that this is verging on a little obsessive as the program works and meets the requirements for extensibility, however... is there a pattern that could be applied to solve this problem more effectively? What's right about my implementation? What's inherently wrong with my implementation? What could be done better?

NB. Please don't worry about the implementation of the Provider.Load() methods - it's the pattern I'm concerned about here.

class Program
{
    static void Main(string[] args)
    {
        string xmlFileName = ConfigurationManager.AppSettings["CustomerLeadssXml"];
        string csvFileName = ConfigurationManager.AppSettings["CustomerLeadssCsv"];

        CustomerLeads customerLeadsFromXml = new CustomerLeads(new XmlCustomerLeadsProvider(xmlFileName));
        customerLeadsFromXml.Load();

        OutputCustomerLeads(customerLeadsFromXml);

        Console.WriteLine("");
        CustomerLeads customerLeadsFromCsv = new CustomerLeads(new CsvCustomerLeadsProvider(csvFileName));
        customerLeadsFromCsv.Load();

        OutputCustomerLeads(customerLeadsFromCsv);

        Console.ReadKey();
    }

    static void OutputCustomerLeads(CustomerLeads customerLeads)
    {
        Console.WriteLine("CustomerLeads:");
        foreach (CustomerLead customerLead in customerLeads.GetCustomerLeads())
        {
            Console.WriteLine("{0} {1} - {2}", customerLead.FirstName, customerLead.LastName, customerLead.EmailAddress);
        }

        Console.WriteLine("");
        Console.WriteLine("CustomerLeadsSorted:");
        foreach (CustomerLead customerLead in customerLeads.GetCustomerLeadsSorted())
        {
            Console.WriteLine("{0} {1} - {2}", customerLead.FirstName, customerLead.LastName, customerLead.EmailAddress);
        }
    }
}

---

public class CustomerLeads
{
    private ICustomerLeadsProvider CustomerLeadsProvider { get; set; }

    public CustomerLeads(ICustomerLeadsProvider customerLeadsProvider)
    {
        if (customerLeadsProvider == null)
            throw new ArgumentException("CustomerLeadsProvider cannot be null", "customerLeadsProvider");

        this.CustomerLeadsProvider = customerLeadsProvider;
    }

    public void Load()
    {
        this.CustomerLeadsProvider.Load();
    }

    public List<CustomerLead> GetCustomerLeads()
    {
        if (this.CustomerLeadsProvider.CustomerLeads == null)
            return null;

        return this.CustomerLeadsProvider.CustomerLeads.ToList<CustomerLead>();
    }

    public List<CustomerLead> GetCustomerLeadsSorted()
    {
        if (this.CustomerLeadsProvider.CustomerLeads == null)
            return null;

        return (from c in this.CustomerLeadsProvider.CustomerLeads
               orderby c.LastName, c.FirstName, c.EmailAddress
               select new CustomerLead()
               {
                   FirstName = c.FirstName,
                   LastName = c.LastName,
                   EmailAddress = c.EmailAddress
               }).ToList<CustomerLead>();
    }
 }

---

public interface ICustomerLeadsProvider
{
    IEnumerable<CustomerLead> CustomerLeads { get; set; }
    void Load();
}

---

public abstract class CustomerLeadsProvider : ICustomerLeadsProvider
{
    public IEnumerable<CustomerLead> CustomerLeads { get; set; }

    public abstract void Load();

    protected bool IsValidFirstName(string firstName)
    {
        return !string.IsNullOrEmpty(firstName);
    }

    protected bool IsValidLastName(string lastName)
    {
        return !string.IsNullOrEmpty(lastName);
    }

    protected bool IsValidEmail(string emailAddress)
    {
        try
        {
            var mailAddress = new System.Net.Mail.MailAddress(emailAddress);
            return true;
        }
        catch
        {
            return false;
        }
    }
}

---

public class XmlCustomerLeadsProvider : CustomerLeadsProvider
{
    private string FileName { get; set; }
    private XDocument CustomerLeadsXDocument { get; set; }

    public XmlCustomerLeadsProvider(string fileName)
    {
        if (string.IsNullOrEmpty(fileName))
            throw new ArgumentException("FileName cannot be null or empty.", "fileName");

        this.FileName = fileName;
    }

    public override void Load()
    {
        this.CustomerLeadsXDocument = XDocument.Load(FileName);

        base.CustomerLeads = from c in CustomerLeadsXDocument.Descendants("CustomerLead")
                             where base.IsValidFirstName((string)c.Descendants("FirstName").FirstOrDefault())
                             && base.IsValidLastName((string)c.Descendants("LastName").FirstOrDefault())
                             && base.IsValidEmail((string)c.Descendants("Email").FirstOrDefault())
                             select new CustomerLead()
                             {
                                 FirstName = (string)c.Descendants("FirstName").FirstOrDefault(),
                                 LastName = (string)c.Descendants("LastName").FirstOrDefault(),
                                 EmailAddress = (string)c.Descendants("Email").FirstOrDefault()
                             };
    }
}

---

public class CsvCustomerLeadsProvider : CustomerLeadsProvider
{
    private string FileName { get; set; }

    public CsvCustomerLeadsProvider(string fileName)
    {
        if (string.IsNullOrEmpty(fileName))
            throw new ArgumentException("FileName cannot be null or empty.", "fileName");

        this.FileName = fileName;
    }

    public override void Load()
    {
        this.CustomerLeads = from l in File.ReadAllLines(this.FileName).Skip(1)
                             let c = l.Split(',')
                             where base.IsValidFirstName(c[0])
                             && base.IsValidLastName(c[1])
                             && base.IsValidEmail(c[2])
                             select new CustomerLead
                             {
                                 FirstName = c[0],
                                 LastName = c[1],
                                 EmailAddress = c[2]
                             };
    }

}

---

<?xml version="1.0" encoding="utf-8"?>
<CustomerLeads>
    <CustomerLead>
        <FirstName>Paul</FirstName>
        <LastName>Smith</LastName>
        <Email>[email protected]</Email>
    </CustomerLead>
    <CustomerLead>
        <FirstName>Nicole</FirstName>
        <LastName>Farhi</LastName>
        <Email>[email protected]</Email>
    </CustomerLead>
    <CustomerLead>
        <FirstName>Raf</FirstName>
        <LastName>Simons</LastName>
        <Email>[email protected]</Email>
    </CustomerLead>
</CustomerLeads>

---

FirstName,LastName,Email
Paul,Smith,[email protected]
Nicole,Farhi,[email protected]
Raf,Simons,[email protected]

Answer 1

What's right about my implementation?

You are adhering to OCP principle, which is good - you can extend providers easily without changing other code.

What's inherently wrong with my implementation?

Your current implementation is good. You need some improvements, but nothing inherently wrong here.

What could be done better?

First thing which looks confusing to me is Load() method of provider:

• Somebody can forget to load data before reading CustomerLeads (I don't like methods which depend on other methods calls). At least you should throw something like InvalidOperationException if someone tries to get data from non-initialized provider.
• Loading all entities to provider and working with them can make your data obsolete, if files will be edited after you loaded data to provider. Especially that becomes problem if you will decide to keep data in shared database.

Also there is a problem with setter of CustomerLeads property. I would expect that setting them back to provider will update my data in file. But actually I just change in-memory collection.

So, I would remove Load() from provider interface, remove setter for CustomerLeads and also renamed provider to repository, because you are providing data, and repository is more appropriate term for that:

public interface ICustomerLeadRepository
{
    IEnumerable<CustomerLead> GetAll();
}

Next is data verification. I think the best class wich knows what is appropriate values for first name, last name and email is CustomerLead itself. There is several options - one is adding guard conditions to each property setter, which will check if value is valid and throw exception if value is not valid:

public class CustomerLead
{
    private string firstName;

    public string FirstName
    {
        get { return firstName; }
        set {
            if (String.IsNullOrEmpty(value))
                throw new ArgumentException();

            firstName = value;
        }
    }

    // ...
}

Also you are missing very important part - you should log/notify about incorrect data in your datasource. If you have guard conditions, then parsing will throw exception rather than skipping incorrect data.

So, implementation of repository can look like:

public class CustomerLeadsXmlRepository : ICustomerLeadRepository
{
    private XDocument xdoc;

    public CustomerLeadsXmlRepository(string fileName)
    {
        xdoc = XDocument.Load(fileName);
    }

    public IEnumerable<CustomerLead> GetAll()
    {
         // reload xdoc if file was modified

         return from c in xdoc.Root.Elements("CustomerLead")
                select new CustomerLead {
                     FirstName = (string)c.Element("FirstName"),
                     LastName = (string)c.Element("LastName")
                     EmailAddress = (string)c.Element("EmailAddress")
                }; 
    }
}

And last thing I would change is CustomerLeads class. I don't see any good reason for its existence - you can sort sequence of objects easily, especially if you will override Equals and GetHashCode methods of CustomerLead class.

Answer 2

For that I would just define a clean abstract interface class (lets say DataSource) and use Polymorhism.

A factory will be good if you need more than one form of the same concrete type now and then and creating an object manually would be too stressfull (there are also other reasons for a factory like tracking the number of sources etc.). This really depends on the needs of your program and the big picture.

Having a design pattern, where it does not make the situation clearer to understand is no good.

Good naming, good specification to UML transformation, avoidance of code smells, KISS and SOLID are more important if you ask me. Master these and you are hell of a good programmer!

Answer 3

I have no idea which (if any) design pattern I have used (possibly abstract factory)!

You may be using Strategy pattern: implementing CustomerLeads methods using an abstract ICustomerLeadsProvider strategy.

I really don't like the implementation as it doesn't feel right.

The following changes would make the code simpler to use, simpler to implement, and/or faster to run:

• Instead of calling an explicit Load method, you could make Load private and invoke it automatically from the constructor.
• Instead of passing-in the filename you could invoke the ConfigurationManager.AppSettings method from inside the constructor.
• CustomerLeads doesn't do much (doesn't add much value): it mostly only delegates to the corresponding ICustomerLeadsProvider method. So you could get rid of CustomerLeads completely, and define its methods (e.g. GetCustomerLeads) in the CustomerLeadsProvider class instead.
• You could get rid of the ICustomerLeadsProvider interface and just use the abstract CustomerLeadsProvider class instead.
• You're storing the result of a Linq expression in the CustomerLeads property which is of type IEnumerable. Does that mean that the Linq expression is re-evaluated every time you iterate through the enumerable? If so is that what you want (e.g. because the underlying file contents are changing at run-time? Or would you prefer to iterate once and cache the result in a List<>?
• GetCustomerLeadsSorted is creating new CustomerLead instances. Instead I'd copy existing CustomerLead instances into a new list (so that the same instance appears in both lists), and sort the new list.

In summary you could do this with just three classes; one abstract CustomerLeadsProvider class:

public abstract class CustomerLeadsProvider
{
    private List<CustomerLead> ListCustomerLeads;

    // or return IEnumerable<CustomerLead> so we don't need to copy the list
    public List<CustomerLead> GetCustomerLeads()
    {
        // return a copy of the list because otherwise caller can change the list
        // see http://programmers.stackexchange.com/q/185166/19237 for example
        return new List<CustomerLead>(ListCustomerLeads);
    }

    public List<CustomerLead> GetCustomerLeadsSorted()
    {
        // TODO: return a sorted copy of the list
    }

    // called from constructor of subclass on Load
    protected IEnumerable<CustomerLead> CustomerLeads
    {
        set { ListCustomerLeads = new List<CustomerLead>(value); }
    }

    protected bool IsValidFirstName(string firstName)
    {
        return !string.IsNullOrEmpty(firstName);
    }

    protected bool IsValidLastName(string lastName)
    {
        return !string.IsNullOrEmpty(lastName);
    }

    protected bool IsValidEmail(string emailAddress)
    {
        try
        {
            var mailAddress = new System.Net.Mail.MailAddress(emailAddress);
            return true;
        }
        catch
        {
            return false;
        }
    }
}

... and two corresponding subclasses.