patterncsharpMinor
Factory pattern for creating sub classes derived from interface with generic type
Viewed 0 times
fromgenericfactorycreatingwithsubtypeinterfaceforclasses
Problem
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
Factory
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.
Context
public interface IPlayerStatsModel
{
string type {get; set;}
}
// concrete implementation
public class BasketballPlayerStats : IPlayerStatsModel
{
// various class members with getters and setters
}
public interface IPlayerStatsManager where T : IPlayerStatsModel
{
Task GetPlayerGameStats(....);
Task> GetPlayersGameStats(...);
// insert and update methods
}
//concrete implementation
public class BasketballPlayerStatsManager : IPlayerStatsManager
{
public BasketballPlayerStatsManager(DbContext context)
{
}
public async Task GetPlayerGameStats(...)
{
}
}Factory
//Factory class
public class PlayerStatsFactory
{
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.
Solution
You have:
And then:
Why not just take a
An abstract factory is a great tool, especially with Dependency Injection. The problem is that yours is a
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...
The cast to interface is also superfluous - if
"Manager" sounds like a very general and not-so-precise way of referring to what other programmers would expect to be called a
You're returning a reference type - the default value can only be
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;Code Snippets
public static IPlayerStatsManager Create(DbContext context, int sportType)if (sportType == (int)Utils.SportType.Basketball)return new BasketballPlayerStatsManager();return default(IPlayerStatsManager);return null;Context
StackExchange Code Review Q#84366, answer score: 3
Revisions (0)
No revisions yet.