Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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)

share|improve this question
    
Your code doesn't compile, your generic type T is used both for IFileRepository and FileRepository –  TopinFrassi Oct 15 '14 at 18:18
    
If T overrides ToString, the whole thing falls apart... –  Mat's Mug Oct 15 '14 at 18:19
    
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 Oct 15 '14 at 18:23
    
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. –  NKD Oct 15 '14 at 18:27

2 Answers 2

  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.

share|improve this answer
    
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 Oct 15 '14 at 20:41
    
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. –  NKD Oct 15 '14 at 20:52
    
@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 Oct 15 '14 at 21:56
    
@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. –  ChrisWue Oct 15 '14 at 21:59
    
You're right. Checking for T is null or empty is unnecessary. Thank you! –  NKD Oct 16 '14 at 16:19

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.

share|improve this answer
    
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 Oct 15 '14 at 21:07
    
I tried this out and I don't have access to fileRepo (repo.fileRepo.SomeMethod();) and sqlRepo (repo.fileRepo.SomeMethod();) –  NKD Oct 15 '14 at 21:15
    
Yup, they'd have to be part of the interface. –  Jesse C. Slicer Oct 15 '14 at 21:21
    
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(); –  NKD Oct 16 '14 at 16:24

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.