I have a problem ...factory

Question

I'm creating a system to generate math problems. As you know in mathematics, there are several different kinds of problems: Binary problems, fractions, decimals, comparating two numbers, etc.

I'm creating an abstract problem factory, where this one is like a black box, which receives a Configuration as input, and returns the Problem as output.

• Problem: This contains the properties which needs the problem generated.
• Configuration: This is the range parameters or conditions to generate a Problem.
• Factory: He is in charge of creating the new Problem.

Here I have my factory interface and marker interfaces:

public abstract class Problem { }
public abstract class Configuration { }

public interface IProblemFactory
{
    Configuration Configuration { get; set; }
    Problem CreateProblem();
}

This is a base class for factories, because I need the Random class. All my classes which implement this one, must have the same seed, so I have a static instance.

public abstract class ProblemFactoryBase<P, C> : IProblemFactory
    where P : Problem
    where C : Configuration
{
    private const int DEFAULT_SEED = 100;
    protected C _config;
    private static Random _random;

    public ProblemFactoryBase()
    {
        if (_random == null) _random = new Random(DEFAULT_SEED);
    }

    public ProblemFactoryBase(C config)
    {
        _config = config;

        if (_random == null) _random = new Random(DEFAULT_SEED);
    }

    protected Random Random { get { return _random; } }

    public C Configuration
    {
        get { return _config; }
        set { _config = value; }
    }

    #region IProblemFactory Implementation

    Configuration IProblemFactory.Configuration
    {
        get { return _config; }
        set
        {
            C config = value as C;
            if (config == null) throw new InvalidCastException("config");

            _config = config;
        }
    }

    protected abstract P CreateProblem(C config);

    #endregion

    public Problem CreateProblem()
    {
        if (_config == null) throw new InvalidOperationException("config");
        return CreateProblem(_config);
    }

    public static void SetSeed()
    {
        _random = new Random(DEFAULT_SEED);
    }
}

Note that the ProblemFactoryBase<P, C> class is a generic type, and implements the abstract method when the type is constructed.

When I have an implementation of all of this. For example, a module for a BinaryProblems like 2+3 would be:

public class BinaryConfiguration : Configuration
{
    public Range<int> Range1 { get; set; }
    public Range<int> Range2 { get; set; }
    public List<Operators> Operators { get; set; }

    public BinaryConfiguration(Range<int> range1, Range<int> range2, List<Operators> operators)
    {
        this.Range1 = range1;
        this.Range2 = range2;
        this.Operators = operators;
    }

public class BinaryProblem : Problem
{
    public BinaryProblem(decimal x, decimal y, Operators op, decimal response)
    {
        this.X = x;
        this.Y = y;
        this.Response = response;
    }

    public decimal X { get; private set; }
    public decimal Y { get; private set; }
    public decimal Response { get; private set; }
}

public enum Operators
{
    Addition, Substract, Multiplication, Division
}

And the most important part, here is a concrete factory. The class specifies the type parameters for the base generic class. Why? Because I supposed is the best way to implement the concrete values, I mean I don't have to cast any value right now.

 public class BinaryFactory : ProblemFactoryBase<BinaryProblem, BinaryConfiguration>
{
    protected override BinaryProblem CreateProblem(BinaryConfiguration config)
    {
        var x = GenerateValueInRange(config.Range1);
        var y = GenerateValueInRange(config.Range2);

        var index = Random.Next(config.Operators.Count);
        var op = config.Operators[index];

        return new BinaryProblem(x, y, op, x + y);
    }

    private decimal GenerateValueInRange(Range<int> range)
    {
        return Random.Next(range.Min, range.Max);
    }
}

And to implement it is:

        BinaryConfiguration configuration = new BinaryConfiguration() {.. }
        IProblemFactory factory = new BinaryFactory(configuration);
        var a = factory.CreateProblem();

It's doing what I like, because I want to put several factories in an array, but at the same time, the implementation of ProblemFactoryBase is good to me because there's no necessity for casting types.

I'm still learning design patterns. Maybe there are another ways to improve it or I'm not following an advice. What is your feedback?

Answer 1

This duplicated code

public ProblemFactoryBase()
{
    if (_random == null) _random = new Random(DEFAULT_SEED);
}

public ProblemFactoryBase(C config)
{
    _config = config;

    if (_random == null) _random = new Random(DEFAULT_SEED);
}  

can be removed by using constructor chaining and be prettified by using braces {} like so

public ProblemFactoryBase()
{
    if (_random == null) { _random = new Random(DEFAULT_SEED); }
}

public ProblemFactoryBase(C config)
    : this()
{
    _config = config;
}  

---

Because the class level constant DEFAULT_SEED is only used with the static Random _random it should be static readonly. In addition, based on the naming guidelines it should be named using PascalCase casing, see also: https://stackoverflow.com/a/242549/2655508 it should look like so

private static readonly int DefaultSeed = 100;  

Because Random _random and the property Random Random (which shouldn't be named like this, because you shouldn't name a property like its typ) won't be changed except for in the constructor, why don't you make Random _random a protected readonly instead ?

Ok, I have now spotted this

public static void SetSeed()
{
    _random = new Random(DEFAULT_SEED);
}  

which is IMO wrong at least the methodname implies something different to what is done. Either change the name of that method or let the method have a method argument which is then used as seed.

---

About #region IProblemFactory Implementation please read Are Regions an antipattern or code smell ?

If you want to keep this region, you should at least include the Problem CreateProblem() method. Btw, why do you have implicit and explicit implementations of the interface ????

---

This

public C Configuration
{
    get { return _config; }
    set { _config = value; }
}

Configuration IProblemFactory.Configuration
{
    get { return _config; }
    set
    {
        C config = value as C;
        if (config == null) throw new InvalidCastException("config");

        _config = config;
    }
}

is at least somehow strange. The first property belongs only to the factory class itself and the second property is the explicit interface implemented property.

The first property needs to go, because it doesn't help any and isn't adding any value to the code. The only advantage of having this is that you don't need to cast the instance to an IProblemFactory because of the explicit interface implementation.

In the second property setter there is no need to do a soft cast to C because C is already a Configuration.

---

Either the implementation of the concrete BinaryFactory or the sample usage of it is flawed and wouldn't compile because the example usage uses a constructor which the BinaryFactory doesn't provide.

Answer 2

When passing parameters to a public method, you always need to assert that the parameter isn't null, otherwise your code will throw NullReferenceException and nobody likes them.

The generic types C and P should not be named this way. The "non-written" convention is that the default letter is T. That doesn't mean we should use any letters! :) If we check the .Net framework naming, we should rename your parameter type name to TProblem, TConfig.

I think you should switch Problem and Configuration for interfaces. By using an abstract class, you prevent the child to inherit a class that might be more useful, especially considering that there are no properties or methods in both the classes. So, using an interface, every child will be free to decide if they need to implement another class.

In the ProblemFactoryBase class, you duplicate code. There's a SetSeed method that contains the same code that is called in your constructor. You should call SetSeed in the constructor instead of calling the same code.

By the way, is this SetSeed method really useful? The _random variable is private, so no one else than you can decide to set it to another value. But you don't change the seed of _random, so why expose a public static method for this? I don't think you need that method. And instead of exposing protected Random, why don't you expose a method GetRandomNumber() that would return _random.Next(). I think that would be clearer. Also, are you sure Random isn't an implementation detail? Why would Random be in your base class? What if the child don't want to use Random? I think Random should be left out of the base class, the child will create their own if they need it.

Do you really need this property :

public C Configuration
{
    get { return _config; }
    set { _config = value; }
}

There's already the interface implementation. This could lead to some confusion. I'm pretty sure that exposing only the interface implementation would be enough! If you're worried about needing to cast your Configuration and not knowing the type, I'd need to ask you : When would you need to cast your Configuration from outside the ProblemFactoryBase (Which knows the type of Configuration, thanks to TConfig)?

The InvalidOperationException message could be clearer. After all, explaining that "the configuration must not be null" isn't that much of a long message and it's much clearer than "config".

I'm not sure you're using the best design pattern to deal with your problem. After all, Problem and Configuration share absolutely nothing. Problem is dependant of Configuration. Since Configuration isn't created via the factory, we'll start by removing it from here. After all, why would a ProblemFactoryBase need a Configuration as a parameter?

Argument #1 : If the Problem class or the CreateProblem doesn't need a Configuration class, it means the Configuration is an implementation detail. Interfaces/abstract classes shouldn't show implementation details. To show my point, here's an example. Imagine one day you have two child classes of ProblemFactoryBase. One which uses a Configuration and one that uses IFooBarAlgorithmService. Will you add IFooBarAlgorithmService to your ProblemFactoryBase? Would you add parameters like this for all the child classes? The ProblemFactoryBase class would end up being a mess.

Argument #2 : If Configuration is a property because you didn't want it to be a parameter of the CreateProblem method, what would happen if you created a problem that didn't need a Configuration? The CreateProblem method would throw InvalidOperationException. But is it really invalid? No. The child class didn't want to use Configuration, and shouldn't need to use it, as it isn't a parameter of the CreateProblem method.

So now that you see why the Configuration property isn't the best move, let's see the solutions :

Either you completely remove the Configuration property from the ProblemFactoryBase class, because it is an implementation detail; or you add the Configuration as a parameter for the CreateProblem public method.

If you think Configuration will be used for every problem created, it should be a parameter of the method, otherwise it should be left out from the base class.

Right now, I think Configuration should be a parameter of your method CreateProblem, let's check all that out. First, we create an interface :

(Note : I think ProblemConfiguration would be a better name than just Configuration, it's less confusing! And for the following examples, I will apply the review I made above)

public interface IProblemFactory<TProblem, TConfig> 
    where TProblem : IProblem 
    where TConfig : IProblemConfiguration
{
    TProblem CreateProblem(TConfig configuration);
}

That interface is pretty simple. It states "Anything that implements me should be able to create a problem using a configuration". That's exactly what we want. Always try to keep your interfaces as clear and concise as possible! (Your previous interface was fine, it's just a tip!)

Next, what would an implementation look like?

public class BinaryProblemFactory : IProblemFactory<BinaryProblem, BinaryConfiguration>
{
    private static readonly Random Random = new Random();

    public BinaryProblem CreateProblem(BinaryConfiguration configuration)
    {
        if (configuration == null) throw new ArgumentNullException(nameof(configuration));

        var x = GenerateValueInRange(configuration.Range1);
        var y = GenerateValueInRange(configuration.Range2);

        var index = Random.Next(configuration.Operators.Count);
        var op = configuration.Operators[index];

        return new BinaryProblem(x, y, op, x + y);
    }

    private static decimal GenerateValueInRange(Range<int> range)
    {
        return Random.Next(range.Min, range.Max);
    }
}

It's clean, and looks a lot like your previous class, but notice there is no base class between the interface and this class and it didn't create much more code (We use the Random variable which might have been used in another subclass, or not!)

Now, you'll have multiple sub classes of IProblemFactory. Wouldn't it be great to have one entry door where we could give a configuration and get the corresponding problem? Oh yeah, that would be neat. That's where the Abstract Factory comes in!

Consider it a wrapper class around all your "sub" factories.

What's cool about it is that it can implement the same existing interface!

public class ProblemFactory : IProblemFactory<IProblem, IProblemConfiguration>
{
    public IProblem CreateProblem(IProblemConfiguration configuration)
    {
        if (configuration == null) throw new ArgumentNullException(nameof(configuration));

        if (configuration.GetType() == typeof (BinaryConfiguration))
        {
            return new BinaryProblemFactory().CreateProblem((BinaryConfiguration)configuration);
        }
        //else if(configuration.GetType() == typeof(SomeOtherConfig)
        //    return new FooProblemFactory().CreateProblem((FooConfiguration)configuration);

        throw new InvalidOperationException("The configuration type isn't mapped to a problem");
    }
}

Boom, you now have a common place to get all your factories. You'll have to add lots of if/else though, as the time passes. We could fix this by changing our interface, which would change some things in the structure of our program, but this solution also has drawbacks. I'll show it to you anyway, so you can decide which fits your need better.

So if our interface was instead :

(Note : While typing, I realized that CreateProblem should be named Create. After all, you already know you're about to create a problem since the return type is IProblem)

public interface IProblemFactory
{
    IProblem Create(IProblemConfiguration configuration);
}

The drawback happens in your child classes :

public class BinaryProblemFactory : IProblemFactory
{
    private static readonly Random Random = new Random();

    private static decimal GenerateValueInRange(Range<int> range)
    {
        return Random.Next(range.Min, range.Max);
    }

    public IProblem Create(IProblemConfiguration configuration)
    {
        if (configuration == null) throw new ArgumentNullException(nameof(configuration));

        //Here, we need to cast and verify for null, that's the drawback.
        //To "counter" it, the parameter xml comments should state we await a BinaryConfiguration
        var binaryConfig = configuration as BinaryConfiguration;

        if (binaryConfig == null) throw new ArgumentException($"{nameof(binaryConfig)} should be of type {nameof(BinaryConfiguration)}");

        var x = GenerateValueInRange(binaryConfig.Range1);
        var y = GenerateValueInRange(binaryConfig.Range2);

        var index = Random.Next(binaryConfig.Operators.Count);
        var op = binaryConfig.Operators[index];

        return new BinaryProblem(x, y, op, x + y);
    }
}

But we gain one advantage in your ProblemFactory :

public class ProblemFactory : IProblemFactory
{
    private readonly Dictionary<Type, IProblemFactory> _factoryMap;

    public ProblemFactory()
    {
        _factoryMap = new Dictionary<Type, IProblemFactory>();
        _factoryMap.Add(typeof (BinaryConfiguration), new BinaryProblemFactory());
    }

    public IProblem Create(IProblemConfiguration configuration)
    {
        if (configuration == null) throw new ArgumentNullException(nameof(configuration));

        IProblemFactory factory;
        bool exists = _factoryMap.TryGetValue(configuration.GetType(), out factory);

        if (!exists)
        {
            throw new InvalidOperationException("The configuration type isn't mapped to a problem");
        }

        return factory.Create(configuration);
    }
}

We can now use a Dictionary instead of having chains of if.

There's also a third solution, that I don't really like, honestly. It involves reflection, meaning it is slower and well... not really clean. I'll show you anyway because well, I'm no master of you. :p This solution works using the first interface. Don't forget that using reflection, you loose the compile time check, which is not a good solution.

public class ProblemFactory : IProblemFactory<IProblem, IProblemConfiguration>
{
    private readonly Dictionary<Type, object> _factoryMap;

    public ProblemFactory()
    {
        _factoryMap = new Dictionary<Type, object>();
        _factoryMap.Add(typeof(BinaryConfiguration), new BinaryProblemFactory());
    }

    public IProblem Create(IProblemConfiguration configuration)
    {
        if (configuration == null) throw new ArgumentNullException(nameof(configuration));

        object factory;
        bool exists = _factoryMap.TryGetValue(configuration.GetType(), out factory);

        if (!exists)
        {
            throw new InvalidOperationException("The configuration type isn't mapped to a problem");
        }

        return (IProblem)factory.GetType().GetMethod("Create").Invoke(factory, new object[] { configuration } );
    }
}

It's up to you to decide which solution is the best between the first and the second as they both have advantages and disadvantages, but both of them should be pretty solid! :)

Answer 3

This doesn't seem like factory pattern should handle. Factory pattern is more like creating objects without exposing the instantiation logic. It's best to use interfaces with this as well (like you have).

Something I think you should check out is interpreter pattern. It's perfect for handling expressions (or problems in this case).

Check this out:

http://www.dofactory.com/Patterns/PatternInterpreter.aspx#_self2

Answer 4

Minor addition to the comments already made:

private decimal GenerateValueInRange(Range<int> range)
{
    return Random.Next(range.Min, range.Max);
}

1. Why is this mixing decimal and int? It seems to me that if the generator generates problems involving ints, it shouldn't use decimals anywhere; and if it generates problems involving decimals, it would be clearer to use Range<decimal> for the bounds.

2. Why is this a private method in a generator? I can make an argument for including it in Random (or a wrapper around Random); I can make an argument for including it in Range<T> if you can find a way to do it which doesn't run into problems with the type system; and I can make an argument for including it as an extension method to Range<int>. But if it's a private method in a generator, you're going to end up copy-pasting it all over the place.

Answer 5

I'm going to focus on one thing as other answers and comments have addressed most of it.

Using a constant seed to create your random instance is madness... It's not random at all, it will produce the same pseudorandom sequence every time you run your application. That seems to be a pretty massive flaw to me!

That's why the default Radom constructor defaults to using some measure of the current time to seed itself - each time you create one it will produce a different sequence (as long as you create them far enough apart).