Generic data interface

Question

I currently have the following working code. I am looking for some suggestion with best practices and perhaps a better way to accomplish my goal.

Goal: - in short - Have a generic data interface as a single data access point to different data sources.

Concern: My current code is working fine but I have to call GetRepository() in each method inside my test class to access the desire instance of the data source repository.

Code Snippet:

public class GenericRepository : IGenericRepository
{
 public T GetRepository<T>() 
 {
 string typeName = typeof(T).ToString();
 if (string.IsNullOrEmpty(typeName))
 return default(T);
 Type repoType = Type.GetType(typeName);
 object repoInstance = Activator.CreateInstance(repoType);
 return (T)(object)repoInstance;
 }
 // more code... 
}

Usage:

public class MyTestClass
{
 IGenericRepository repo = new GenericRepository();
 private void DoThis()
 {
 IFileRepository repoA = repo.GetRepository<FileRepository>();
 // now I can call repository A interface methods like this repoA.SomeMethod()
 }
 private void DoThat()
 {
 ISQLRepository repoB = repo.GetRepository<SQLRepository>();
 // now do something with repository B interface methods like this repoB.SomeMethod()
 }
}

I thought of an alternative way to do this but it has its own drawbacks as well.

1. Implement the interface methods in both IGenericeRepository and as well as the interface I inherited.

2. Implement the methods for inherited interface inside IGenericRepository and put it in each different region and end up with a massive page of code

Thoughts? Comments? Suggestion? -- I personally think the first option is a better choice but wonder if there are better ideas or maybe there is something wrong with my implementation.

Alternative Code

public class GenericRepository : IGenericRepository, IFileRepository, ISQLRepository
{
 public FileRepository fileRepo { get; set; }
 public SQLRepository sqlRepo { get; set; }
 public GenericRepository()
 {
 fileRepo = new FileRepository();
 sqlRepo = new SQLRepository();
 }
 // more code... 
}

Alternative Usage

public class MyTestClass
{
 IGenericRepository repo = new GenericRepository();
 private void DoThis()
 {
 // now I can access file repository like this: repo.fileRepo.SomeMethod();
 // now I can access sql repository methods like this: repo.sqlRepo.SomeMethod();
 }
 private void DoThat()
 {
 // now I can access file repository like this: repo.fileRepo.SomeMethod();
 // now I can access sql repository methods like this: repo.sqlRepo.SomeMethod();
 }
}

====== updated:

To provide more context. Below is the solution structure

-- Solution

---- Repository Project (GenericRepository, FileRepository, SQLRepository)

---- IRepository Project (IGenericRepository, IFileRepository, ISQLRepository)

---- MyTestProject Project (MyTestClass)

• IEatBagels: Your code doesn't compile, your generic type T is used both for IFileRepository and FileRepository
• Mathieu Guindon: If T overrides ToString, the whole thing falls apart...
• NKD: I didn't post the FileRepository code. Did you create a dummy FileRepository class as a place holder? Are you trying to compile option 1 or alternative code?
• NKD: T is of type FileRepository or SQLRepository class -- whichever you pass it as T parameter. The code works for me. Maybe I miss post something. Let me know if you what need from me.

Answer 1

1. Your current implementation is somewhat flawed as relying on ToString to get the type name is dangerous. ToString is meant largely to yield or short meaningful description of the object for debugging, logging etc.

2. typeof(T) already yields a Type - it is totally unnecessary to convert it to a string just to then turn it back into a type again.

3. If your repositories all have a parameterless default constructor you can avoid calling CreateInstance entirely.

Your current implementation can be shortened to:

public T GetRepository<T>() 
{
 object repoInstance = Activator.CreateInstance(typeof(T));
 return (T)(object)repoInstance;
}

or possibly even to:

public T GetRepository<T>() where T : new()
{
 return new T();
}

In addition to that your GenericRepository seems more like a repository factory than a repository in itself hence you should possibly rename it to RepositoryFactory and the method to CreateRepository.

• NKD: Thank you, Chris. Your inputs make a lot of sense! Before I went ahead with this though - will it still be a true Factory if I have methods which live inside the RepositoryFactory returning datatables consisting of combined data from the FileRepository and SQLRepository? This is the part that get me confused if it should be a "Generic" or a "Factory" repository.
• NKD: Regarding the shorten version of the GetRepository you provided. I did a ToString because I wanted to return some sort of default instance in case T is not passed in and I'm not sure what should be a default instance of T. Maybe I should just throw an exception there.
• ChrisWue: @NKD: In a generic method you do not have the option to "not pass in T". It won't compile if the compiler can't infer the type (i.e. var repo = genericRepoInstance.GetRepository() or var repo = genericRepoInstance.GetRepository<>() won't compile)
• ChrisWue: @NKD: Regarding your first comment: It's hard to answer this without seeing more code. Maybe you should submit the GenericRepository implementation for review (or at least enough to enable a sensible code review). Please ask a separate question if you do so.
• NKD: You're right. Checking for T is null or empty is unnecessary. Thank you!

Answer 2

Better yet, inject your dependencies:

public class MyTestClass
{
 readonly IGenericRepository repo;
 public MyTestClass(IGenericRepository repo)
 {
 if (repo == null)
 {
 throw new ArgumentNullException("repo");
 }
 this.repo = repo;
 }
 private void DoThis()
 {
 // now I can access file repository like this: repo.fileRepo.SomeMethod();
 // now I can access sql repository methods like this: repo.sqlRepo.SomeMethod();
 }
 private void DoThat()
 {
 // now I can access file repository like this: repo.fileRepo.SomeMethod();
 // now I can access sql repository methods like this: repo.sqlRepo.SomeMethod();
 }
}

Then you have decoupled MyTestClass from the concrete GenericRepository implementation. This allows for lesser cohesion and easier unit testing and changing of implementations.

• NKD: Are you saying that the Alternative version is a better way to implement this? And then use the improved code (aka dependencies inject) to access the fileRepo and sqlRepo methods? With dependencies injection, I still have to write code for IFileRepository (i.e. void DoSomething()...) and then write the implementation of void DoSomething() in the GenericRepository. My GenericRepository will eventually contain a lot of code if I have many different data sources. I hope this makes sense as to what I am concerning with. Thank you in advance for your input.
• NKD: I tried this out and I don't have access to fileRepo (repo.fileRepo.SomeMethod();) and sqlRepo (repo.fileRepo.SomeMethod();)
• Jesse C. Slicer: Yup, they'd have to be part of the interface.
• NKD: Thank you. I am now accessing the other repositories like this (see below) without adding additional code to the GenericRepository. //repo.GetRepository<SQLRepository>().DoSomething(); and //repo.GetRepository<FileRepository>().DoSomething();

Answer 3

Your GetRepository which create new instance using reflection is slow. I've once done like that, but I've come up with another design.

I usually create abstract class from that GenericRepository and then Create derived class FileRepository and SQLRepository from that abstract class. This is what it look like.

First the interface is something like this

public interface IGenericRepository
{
 void Add(object item);
 void Edit(object item);
}
public interface ISQLRepository : IGenericRepository
{
 void SaveChanges();
}
public interface IFileRepository : IGenericRepository
{
 void Upload();
}

ISQLRepository as well as IFileRepository implemented IGenericRepository. This way class that implement ISQLRepository will implement a method from IGenericRepository as well. Then the abstract class

public abstract class GenericRepository : IGenericRepository
{
 /// <summary>
 /// you can add any method that should be implemented in derived class like this
 /// </summary>
 /// <param name="item"></param>
 public abstract void Add(object item);
 /// <summary>
 /// or you could just create a generic method
 /// </summary>
 /// <param name="item"></param>
 public virtual void Edit(object item)
 {
 //implement generic method to edit an item
 }
}

The implementation of that abstract class

public class FileRepository : GenericRepository, IFileRepository
{
 public override void Add(object item)
 {
 throw new NotImplementedException();
 }
 public override void Upload()
 {
 throw new NotImplementedException();
 }
}
public class SQLRepository : GenericRepository, ISQLRepository
{
 public override void Add(object item)
 {
 throw new NotImplementedException();
 }
 public override void SaveChanges()
 {
 throw new NotImplementedException();
 }
}

and you use it like this

public class MyTestClass
{
 IFileRepository fileRep = new FileRepository();
 ISQLRepository sqlRep = new SQLRepository();
}

• forsvarir: This site is really aimed at reviewing the code in the question. To bring your answer more on topic, could you expand on why you believe your approach is an improvement?