Design pattern for implementing multiple data sources

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:

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

• 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("

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 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 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.

StackExchange Code Review Q#42133, answer score: 10