I have a problem ...factory

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.

ProblemFactory -> 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 : 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 CreateProble

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.

StackExchange Code Review Q#10532, answer score: 14