First Repo IpersonRepository repository = new ServiceRepository(); var people =repository.GetPeople(); foreach(var person in people) { personListBox.Items.Add(person); } ------------------------------------------ Second Repo IpersonRepository repository = new CSVRepository(); var people =repository.GetPeople(); foreach(var person in people) { personListBox.Items.Add(person); } ------------------------------------- ThirdRepo IpersonRepository repository = new SQLRepositroy(); var people =repository.GetPeople(); foreach(var person in people) { personListBox.Items.Add(person); } ------------------------------------------------- Add new class Repository Factory class ---------------------------------------------- public static class RepositoryFactory { public static IPersonRepository GetRepository(string repositoryType) { IPersonRepository repo =null; switch(repositoryType) { case RepoType.Service: return new ServiceRepository(); case RepoType.CSV: return new CSVRepository(); case RepoType.SQL: return new SQLRepository(); default: throw new ArgumentException("Invalid Repository Type"); } return repo; } } --------------------- public enum RepoType { Service, CSV, SQL } Take Method and Extract to method FetchData Method that takes string argument private void FetchData(string repositoyType) { IpersonRepositroy repositroy = RepositoryFactory.GetRepository(repositroyTpye); var people =repository.GetPeople(); foreach(var person in people) { personListBox.Items.Add(person); } } ---------------------------------- Now going back to the each method and replace the code FetchData( RepoType.Service); FetchData( RepoType.CSV); FetchData( RepoType.SQL);
Good idea with the factory pattern and using interfaces to decouple your classes.
I would suggest 2 things, though:
1. Consider an enum for the repository type instead of a string. While the code only allows for certain string values to retrieve an instance, why not add compile-time checking and force your users from the start to pick the correct instance? Besides, with using strings, what if the user puts in “sql”, “csv”, or “service”? These will fail because the switch statement is case-sensitive. Using an enum will restrict what is allowed and present no ambiguities.
2. Another thing to consider is simply returning from inside each case statement. Replacing “repo = new ServiceRepository(); break;” (and returning it later) with just “return new ServiceRepository()” eliminates the need for break statements (I’m always a fan of removing unneeded clutter).
Keep writing! I wish I had more time to do it. Take care.
Thanks Ryan, I really learned a lot working with you. I will definitely consider your points and do more refactoring of a refactoring. Thanks for your feedback