Factory pattern for creating sub classes derived from interface with generic type

Question

Would like to know if the factory pattern I've implemented for my stat library is the correct way to go. I have an interface IPlayerStatsManager where the type parameter is an interface as well (IPlayerStatsModel). The IPlayerStatsManager is an interface that lists the common operations that can be performed on any class that implements IPlayerStatsModel. I've only completed the implementation for basketball and it seems to work correctly but would like to know if the way I currently have it is the most efficient way. Here's the code (some of the implementation removed for brevity)

Context

public interface IPlayerStatsModel
{
    string type {get; set;}
}

// concrete implementation
public class  BasketballPlayerStats : IPlayerStatsModel
{
    // various class members with getters and setters
}

public interface IPlayerStatsManager<T> where T : IPlayerStatsModel
{
    Task<T> GetPlayerGameStats(....);

    Task<List<T>> GetPlayersGameStats(...);

    // insert and update methods
}

//concrete implementation
public class BasketballPlayerStatsManager : IPlayerStatsManager<BasketballPlayerStats>
{
    public BasketballPlayerStatsManager(DbContext context)
    {
    }

    public async Task<BasketballPlayerStats> GetPlayerGameStats(...)
    {
    }
}

Factory

//Factory class
public class PlayerStatsFactory<IPlayerStatsManager>
{
    public static IPlayerStatsManager Create(DbContext context, int sportType)
    {
        if (sportType == (int)Utils.SportType.Basketball)
        {
             return (IPlayerStatsManager)Activator.CreateInstance(typeof(BasketballPlayerStatsManager));
        }
        return default(IPlayerStatsManager);
     }
}

I feel like there might be a better way but I'm not sure what that would look like. Any pointers would be greatly appreciated.

Answer 1

You have:

public static IPlayerStatsManager Create(DbContext context, int sportType)

And then:

if (sportType == (int)Utils.SportType.Basketball)

Why not just take a SportType parameter and remove that cast?

---

An abstract factory is a great tool, especially with Dependency Injection. The problem is that yours is a static method, which makes it available just about anywhere in the code, effectively hiding dependencies and defeating the purpose of IoC and DI: that factory becomes some sort of ambient context that's there waiting to be used by anyone, and your constructors don't need to document the fact that you're depending on an abstract factory: that smells IMO.

The factory implementation has one purpose: instantiate types. It has to know what it's creating. Your usage of reflection is nothing but a very convoluted way of saying...

return new BasketballPlayerStatsManager();

The cast to interface is also superfluous - if BasketballPlayerStatsManager does implement IPlayerStatsManager, then that's all the client code will see anyway (of course it implements that interface).

---

"Manager" sounds like a very general and not-so-precise way of referring to what other programmers would expect to be called a Repository; I'd rename it as such.

---

return default(IPlayerStatsManager);

You're returning a reference type - the default value can only be null. Your intent would be much clearer like this:

return null;