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.

The factory should only have the responsibility of creating instances and knowing what types of instances to create, without having the added overhead of knowing about configurations.

With that in mind, here is how I would approach the problem:

/// <summary>
/// Each class that can generate a problem should accept a problem configuration
/// </summary>
public class BinaryProblem : IProblem
{
    public BinaryProblem (ProblemConfiguration configuration)
    {
        // sample code, this is where you generate your problem, based on the configuration of the problem
        X = new Random().Next(configuration.MaxValue + configuration.MinValue) - configuration.MinValue;
        Y = new Random().Next(configuration.MaxValue + configuration.MinValue) - configuration.MinValue;
        Answer = X + Y; 
    }

    public int X { get; private set; }
    public int Y { get; private set; }
    public int Answer { get; private set; }
}

For this we will need a problem configuration class

/// <summary>
/// A problem configuration class
/// </summary>
public class ProblemConfiguration
{
    public int MinValue { get; set; }
    public int MaxValue { get; set; }
    public Operator Operator { get; set; }
}

I would also a dedicated class for handling the configuration of levels and remove it from the factory class.

/// <summary>
/// The abstract level configuration allows descendent classes to configure themselves
/// </summary>
public abstract class LevelConfiguration
{
    protected Random Random = new Random();
    private Dictionary<Level, ProblemConfiguration> _configurableLevels = new Dictionary<Level, ProblemConfiguration>();

    /// <summary>
    /// Adds a configurable level.
    /// </summary>
    /// <param name="level">The level to add.</param>
    /// <param name="problemConfiguration">The problem configuration.</param>
    protected void AddConfigurableLevel(Level level, ProblemConfiguration problemConfiguration)
    {
        _configurableLevels.Add(level, problemConfiguration);
    }

    /// <summary>
    /// Removes a configurable level.
    /// </summary>
    /// <param name="level">The level to remove.</param>
    protected void RemoveConfigurableLevel(Level level)
    {
        _configurableLevels.Remove(level);
    }

    /// <summary>
    /// Returns all the configurable levels.
    /// </summary>
    public IEnumerable<Level> GetConfigurableLevels()
    {
        return _configurableLevels.Keys;
    }

    /// <summary>
    /// Gets the problem configuration for the specified level
    /// </summary>
    /// <param name="level">The level.</param>
    public ProblemConfiguration GetProblemConfiguration(Level level)
    {
        return _configurableLevels[level];
    }
}

This would allow the Binary Configuration to look something like this:

/// <summary>
/// Contains level configuration for Binary problems
/// </summary>
public class BinaryLevelConfiguration : LevelConfiguration
{
    public BinaryLevelConfiguration()
    {
        AddConfigurableLevel(Level.Easy, GetEasyLevelConfiguration());
        AddConfigurableLevel(Level.Medium, GetMediumLevelConfiguration());
        AddConfigurableLevel(Level.Hard, GetHardLevelConfiguration());
    }

    /// <summary>
    /// Gets the hard level configuration.
    /// </summary>
    /// <returns></returns>
    private ProblemConfiguration GetHardLevelConfiguration()
    {
        return new ProblemConfiguration
        {
            MinValue = 100,
            MaxValue = 1000,
            Operator = Operator.Addition
        };
    }

    /// <summary>
    /// Gets the medium level configuration.
    /// </summary>
    /// <returns></returns>
    private ProblemConfiguration GetMediumLevelConfiguration()
    {
        return new ProblemConfiguration
        {
            MinValue = 10,
            MaxValue = 100,
            Operator = Operator.Addition
        };
    }

    /// <summary>
    /// Gets the easy level configuration.
    /// </summary>
    /// <returns></returns>
    private ProblemConfiguration GetEasyLevelConfiguration()
    {
        return new ProblemConfiguration
        {
            MinValue = 1,
            MaxValue = 10,
            Operator = Operator.Addition
        };
    }


}

Now the factory should only be responsible for creating new instances of problems and knowing what types of problems it can serve

/// <summary>
/// The only responsibility of the factory is to create instances of Problems and know what kind of problems it can create, 
/// it should not know about configuration
/// </summary>
public class ProblemFactory
{

    private Dictionary<Type, Func<Level, IProblem>> _registeredProblemTypes; // this associates each type with a factory function

    /// <summary>
    /// Initializes a new instance of the <see cref="ProblemFactory"/> class.
    /// </summary>
    public ProblemFactory()
    {
        _registeredProblemTypes = new Dictionary<Type, Func<Level, IProblem>>();
    }

    /// <summary>
    /// Registers a problem factory function to it's associated type
    /// </summary>
    /// <typeparam name="T">The Type of problem to register</typeparam>
    /// <param name="factoryFunction">The factory function.</param>
    public void RegisterProblem<T>(Func<Level, IProblem> factoryFunction)
    {
        _registeredProblemTypes.Add(typeof(T), factoryFunction);
    }

    /// <summary>
    /// Generates the problem based on the type parameter and invokes the associated factory function by providing some problem configuration
    /// </summary>
    /// <typeparam name="T">The type of problem to generate</typeparam>
    /// <param name="problemConfiguration">The problem configuration.</param>
    /// <returns></returns>
    public IProblem GenerateProblem<T>(Level level) where T: IProblem
    {
        // some extra safety checks can go here, but this should be the essense of a factory,
        // the only responsibility is to create instances of Problems and know what kind of problems it can create
        return _registeredProblemTypes[typeof(T)](level); 
    }
}

Then here is how you could use all this

class Program
{
    static void Main(string[] args)
    {
        ProblemFactory problemFactory = new ProblemFactory();
        BinaryLevelConfiguration binaryLevelConfig = new BinaryLevelConfiguration();


        // register your factory functions
        problemFactory.RegisterProblem<BinaryProblem>((level) => new BinaryProblem(binaryLevelConfig.GetProblemConfiguration(level)));

        // consume them
        IProblem problem1 = problemFactory.GenerateProblem<BinaryProblem>(Level.Easy);
        IProblem problem2 = problemFactory.GenerateProblem<BinaryProblem>(Level.Hard);
    }
}

The problem is, in BinaryProblemConfiguration methods, I'd like to add more configurations in the methods, I mean, returns something like an IEnumerable, but I'm not sure how would I modify ProblemFactory.

I guess I need to create another class ListProblemFactory, and maintain the last configuration to show the next one.

I mean generate problems with the configs sequently.

share|improve this question
3  
I've been following your ongoing quest for this for most of your posts. I may be out of line here, but I just have to say: Do you REALLY need this level of abstraction? I get the feeling that we're currently at the ProblemConfigurationProblemLevelConfigurationFactoryFactory level. –  Willem van Rumpt Feb 4 '12 at 19:18
1  
"BinaryProblemConfiguration" doesn't exist in any of the code you posted. What exactly are you referring to there? –  Brannon Jun 27 '12 at 4:34
    
@Brannon He's probably refering to BinaryLevelConfiguration. –  Quentin Pradet Sep 20 '12 at 9:57
    
I'm confused. Can you give example code of adding "more configurations in the methods"? –  tallseth Feb 19 '13 at 2:36
add comment

closed as unclear what you're asking by Mat's Mug, Malachi, Brian Reichle, Jamal, svick Nov 17 '13 at 11:20

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question.If this question can be reworded to fit the rules in the help center, please edit the question.

2 Answers

Sure, you can do that, why not? Isn't it easy to pass a list of Levels instead of a Level in GenerateProblem? Without more details on what you really want to do with that list, it's hard to answer.

Unfortunately, as Willem van Rumpt said in comments, we're currently at the ProblemConfigurationProblemLevelConfigurationFactoryFactory level and it doesn't make sense anymore. You need to learn when to stop abstracting things.

share|improve this answer
add comment

The original question is too old, so adding my two cents for future comers :)

It seem that OP is using a Service Locator pattern.


/// <summary>
/// Each class that can generate a problem should accept a problem configuration
/// </summary>
public class BinaryProblem : IProblem
{
    readonly ProblemConfiguration _configuration;
    public BinaryProblem(ProblemConfiguration configuration)
    {
        _configuration = configuration;
    }

    public void Generate() 
    {
        // sample code, this is where you generate your problem, based on the configuration of the problem
        X = new Random().Next(_configuration.MaxValue + _configuration.MinValue) - _configuration.MinValue;
        Y = new Random().Next(_configuration.MaxValue + _configuration.MinValue) - _configuration.MinValue;
        Answer = X + Y;
    }

    public int X { get; private set; }
    public int Y { get; private set; }
    public int Answer { get; private set; }
}

improvement in the above code fragment:

  1. extracted out a Generate method from constructor, conceptually speeking, constructor is responsible for initialising an object and nothing more than that. It is a best practice, can't say about the Random class and testability
  2. As posted by the OP's code it seems that LevelConfiguration class doesn't solve any purpose. Also, if it does it should be better renamed to Level_ProblemConfigurationStore in my opinion as that is what it actually does.

/// <summary>
/// Contains level configuration for Binary problems
/// </summary>
public class BinaryLevelConfiguration
{
    public ProblemConfiguration GetProblemConfiguration(Level level)
    {
        switch (level)
        {
            case Level.Easy:
                return GetEasyLevelConfiguration();
            case Level.Medium:
                return GetMediumLevelConfiguration();
            case Level.Hard:
                return GetHardLevelConfiguration();
            default: //throw not implemented here
                return null;
        }
    }
    /// <summary>
    /// Gets the hard level configuration.
    /// </summary>
    /// <returns></returns>
    private ProblemConfiguration GetHardLevelConfiguration()
    {
        return new ProblemConfiguration
        {
            MinValue = 100,
            MaxValue = 1000,
            Operator = Operator.Addition
        };
    }

    /// <summary>
    /// Gets the medium level configuration.
    /// </summary>
    /// <returns></returns>
    private ProblemConfiguration GetMediumLevelConfiguration()
    {
        return new ProblemConfiguration
        {
            MinValue = 10,
            MaxValue = 100,
            Operator = Operator.Addition
        };
    }

    /// <summary>
    /// Gets the easy level configuration.
    /// </summary>
    /// <returns></returns>
    private ProblemConfiguration GetEasyLevelConfiguration()
    {
        return new ProblemConfiguration
        {
            MinValue = 1,
            MaxValue = 10,
            Operator = Operator.Addition
        };
    }

}

Improvement:

  1. As per the OP's code and my previous comments, inheritance is not required, indeed this is a Factory class as You are creating/ configuring something here.

  2. You are configuring the dictionary in Your constructor, consider extracting that part as shown in the above code if required. I believe You don't even require a dictionary here


Finally the calling code looks like this:

    static void Main(string[] args)
    {
        ProblemFactory problemFactory = new ProblemFactory();
        BinaryLevelConfiguration binaryLevelConfig = new BinaryLevelConfiguration();


        // register your factory functions
        problemFactory.RegisterProblem<BinaryProblem>((level) =>
        {
            var binProblem = new BinaryProblem(binaryLevelConfig.GetProblemConfiguration(level));
            binProblem.Generate();
            return binProblem;
        });

        // consume them
        IProblem problem1 = problemFactory.GenerateProblem<BinaryProblem>(Level.Easy);
        IProblem problem2 = problemFactory.GenerateProblem<BinaryProblem>(Level.Hard);

    }

A word of Caution: You can easily forget to call Generate method of binary problem, You should be having appropriate tests to ensure that.

Note: Since the OP never clarified the question as per comments, I assume that he wishes to add new level of difficulties, You can easily do that by modifying the BinaryLevelConfiguration Class.

share|improve this answer
add comment

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