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.

I'm creating a factory design for math problems. The idea which I have is:

  • When the factory initializes, creates in a list some problems (20 initially)
  • If the program wants more than 20, the list should grow until reach the requested quantity.

For example if I require 30 problems of the X problem, will generate two times.

  • Some problems must be generated more difficult. I pass in the constructor the level and the factory must generate them.
  • To do this, I've got an abstract method called ConfigureLevels, where you has to setup.
  • I set an abstract method called Generate, this one must be implemented in a concrete class.
  • When the problem is generated, sometimes is not a good problem. When this happen, it must generate anothers until gets a good problem. A good problem is according to a CRITERIA. I mean, the problem musn't exist in the list, if we refer that the factory generates FRACTIONS, must be below than 1, etc. And I haven't done yet this last feature.

This is the factory which I'm talking to.

public abstract class Factory<T> where T : IProblem
{
    protected static Random rnd;
    protected LinkedListNode<T> actualNode;
    protected LinkedList<T> problems;

    public virtual int TotalProblemsPossible { get; set; }
    protected virtual int InitialSize { get; set; }

    protected Factory(Levels level)
    {
        this.InitialSize = 20;

        Initialize();
        ConfigureLevels(level);
        FirstTime();
    }

    public virtual void FirstTime()
    {
        if (rnd == null) rnd = new Random(100);
        if (problems == null)
        {
            problems = new LinkedList<T>();
            Generate(problems);
        }

        actualNode = problems.First;
    }

    public virtual T CreateProblem()
    {
        if (actualNode.Next == null)
        {
            Generate(problems);
        }

        actualNode = actualNode.Next;
        return actualNode.Value;
    }

    private void Generate(LinkedList<T> problems)
    {
        for (int i = 0; i < InitialSize; i++)
        {
            T newProblem;
            int bucles = 0;

            do
            {
                if (bucles == 50) throw new InvalidOperationException();

                newProblem = Generate();
                bucles++;
            } while (problems.Contains(newProblem));

            problems.AddLast(newProblem);
        }
    }

    protected virtual void Initialize()
    {
    }

    protected abstract T Generate();
    protected abstract void ConfigureLevels(Levels level);
}

And here is a concrete class, check how I'm overriding some methods from the abstract Factory. The factory create times table problems, and it knows how to calculates.

public class TimesTableProblemFactory : Factory<BinaryProblem>
{
    private Bound<int> Bound1 { get; set; }
    private Bound<int> Bound2 { get; set; }

    public TimesTableProblemFactory(Levels level)
        : base(level)
    {
    }

    protected override void Initialize()
    {
        base.Initialize();
    }

    protected override void ConfigureLevels(Levels level)
    {
        switch (level)
        {
            case Levels.Easy:
                this.Bound1 = new Bound<int>(2, 6);
                this.Bound2 = new Bound<int>(3, 11);

                break;
            case Levels.Normal:
                this.Bound1 = new Bound<int>(3, 13);
                this.Bound2 = new Bound<int>(3, 10);

                break;
            case Levels.Hard:
                this.Bound1 = new Bound<int>(6, 13);
                this.Bound2 = new Bound<int>(3, 10);

                break;
        }
    }

    protected override BinaryProblem Generate()
    {
        BinaryProblem problem;

        int number1 = rnd.Next(Bound1.Min, Bound1.Max);
        int number2 = rnd.Next(Bound1.Min, Bound1.Max);
        Answer<decimal> correctValue = new Answer<decimal>(number1 * number2);

        problem = new BinaryProblem(number1, number2, Operators.Multiplication, correctValue);
        return problem;
    }
}

Here is another factory for the same problem, where creates addings and multiplications.

public class BinaryProblemFactory : Factory<BinaryProblem>
{
    private Bound<int> _bound;
    private LinkedList<Operators> _operatorsList;
    private LinkedListNode<Operators> _node;

    public BinaryProblemFactory(Levels level)
        : base(level)
    {
    }

    protected override void Initialize()
    {
        _bound = new Bound<int>();
        _operatorsList = new LinkedList<Operators>(new List<Operators>() { Operators.Addition, Operators.Multiplication });

        base.Initialize();
    }

    protected override void ConfigureLevels(Levels level)
    {
        switch (level)
        {
            case Levels.Easy:
                this._bound = new Bound<int>(10, 100);

                break;
            case Levels.Normal:
                this._bound = new Bound<int>(50, 200);

                break;
            case Levels.Hard:
                this._bound = new Bound<int>(100, 10000);

                break;
        }
    }

    private Operators NextOperator()
    {
        if (_node == null || _node.Next == null)
        {
            _node = _operatorsList.First;
        }
        else
        {
            _node = _node.Next;
        }

        return _node.Value;
    }

    protected override BinaryProblem Generate()
    {
        BinaryProblem problem;

        int number1 = rnd.Next(_bound.Min, _bound.Max);
        int number2 = rnd.Next(_bound.Min, _bound.Max);
        Answer<decimal> correctValue = new Answer<decimal>(number1 + number2);

        problem = new BinaryProblem(number1, number2, NextOperator(), correctValue);
        return problem;
    }
}

I feel is not a well design, so I need a little of help to organize these factories. I'm gonna create around 200 hundreds of it. So, I prefer fix these intead spent pains.

UPDATE: Until now, the design has improved a lot.

enter image description here

Now I need to know a list of available levels. I guess create three three virtual functions CanConfigureXLevel is not a good way.

Maybe it'll be great create a some dictionary which contains the available levels (Level enum as key) and value should have something like a container of objects usefull to generate the problem (for example binary and times tables both needs two bound objects).

Here is the current project: http://www.mediafire.com/?tlm7abea14tdeks

share|improve this question
    
I fail to see the point of GrowSize, why don't you just generate problems problems as you need them? It's not like it's going to be faster to generate many problems in a row, right? We're not talking about resizing an array here. –  João Portela Jan 31 '12 at 15:11
add comment

1 Answer 1

up vote 3 down vote accepted
+50

I think your list factory shouldn't be responsible for creating the concrete problems. How about separating the list factory from the problem factory? That way you can have timeTableProblems and binaryProblems in the same list. (even though that might not be a requirement, it's still a nice separation of concerns)

It actually looks like you've catered for this since you have the IProblem interface. The list factory wouldn't have to be generic either, it'd just keep a list of IProblems generated by whatever problem factories it's given.

Also, I believe you could apply the template method pattern to some of your methods. There is some repeated code in each of your concretes.

For instance you could pull the ConfigureLevels method up to the base class and call down to ConfigureEasy, ConfigureMedium and ConfigureHard.

Update

Regarding template methods, the point is to remove duplication, and at the same time avoid letting the FirstTime method be overridden for instance. It should call out to an empty OnFirstTime method instead. Derivatives might just forget to call base.FirstTime, and you probably don't want that. (Why not just join it with Initialize() ? )

Here's a stub of a sample of separating the list factory and the problem factories. The point is that these are two different things. A list of problems, and the problems themselves.

Also have a look at this: http://en.wikipedia.org/wiki/Single_responsibility_principle

public class ProblemListFactory
{ 
    // omitted stuff

    protected Factory(ProblemFactory problemFactory) 
    { 
         // ...
    } 

    public void Generate()
    {
        for(// ...)
        {
            IProblem problem = problemFactory.Generate();
            if (ProblemExists(problem))
                continue;
            problems.Add(problem);
        }
    }
}

public abstract class ProblemFactory
{
    protected Random Random = new Random();

    public abstract IProblem Generate();
}

public class BinaryProblemFactory : ProblemFactory
{
    public BinaryProblemFactory(Level level)
    {
        // ...
    }

    public override IProblem Generate()
    {
        // ...
        return new BinaryProblem(x, y, operator, answer);
    }
}

// ...
var binaryProblemListFactory = new ProblemListFactory(new BinaryProblemFactory(Level.Easy));
var binaryProblems = binaryProblemListFactory.Generate();
var timeTableProblemListFactory = new ProblemListFactory(new TimeTableProblemFactory(Level.Medium));
var timeTableProblems = timeTableProblemListFactory.Generate();
share|improve this answer
    
I do not get your point, can you show an example of this (or an interface of it) –  Darf Zon Jan 25 '12 at 17:48
    
I really do not have idea about the design patterns and it's beautiful your design. In fact right now, I'm studying them. For example, I'm trying to implement a singleton for Random property. One doubt, how do you think will be the best way to pass paramaters for configuration. I mean, pass the Bound class of them. –  Darf Zon Jan 27 '12 at 1:16
    
Great. :) Just remember not to use a sledge hammer for nails. I don't think you need a singleton for the random property. Just make it static if you want a common seed. Here's the overview for the siblings of SRP: en.wikipedia.org/wiki/SOLID_(object-oriented_design) You might also be interested in reading the Gang of four Design Patterns book. (google it) –  Lars-Erik Jan 27 '12 at 9:15
    
About the bounds, the factories could either take a dictionary of levels and bounds as parameter, or you could just pass the bounds and ditch the level completely. Although I don't see a problem with having the bounds config in the problem factories as you did with the list factories. –  Lars-Erik Jan 27 '12 at 9:53
    
I think is better because, later I need to show which levels are available, so I guess this is better. I've updated the post and the project, I just want to know how would I need to modified to implement the feature you're telling me (the dictionaries). –  Darf Zon Jan 27 '12 at 19:50
show 4 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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