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 math quiz system. There are so many questions, as you will be able to see in it they're so different and I have a difficult time trying to create the system.

The quiz is generated randomly, many problems of them are differently.

For example:

  • \$1 + 3 = X\$, where \$X\$ is an integer.
  • \$0.5 = X / Y\$, where \$X\$ and \$Y\$ are integers, and they're stored in a vector.
  • \$X <= Y <= Z\$, where \$X\$, \$Y\$, and \$Z\$ are doubles, and they're also stored in an array.

I need to store each student's answer. I've got a Question class and it has a Result property where contains his answer, but its datatype is object, and I think that's not correct.

This is the second problem:

I'm using several user controls (I don't know if this is the best way to do it) to show the questions or problems.

Please, check it out the solution. Any question you have, please let me know. That's what I have.

Ok, this will be the first question for this project.

This is a math quiz system. Always must generate different problems for each problem, but at the same time, I have to generate the same exam for each one (using the same seed in random class).

Many problems are different. There are times tables, addition and subtraction of signed numbers, comparing integers, comparing fractions, convert fraction to decimal, and others I can tell while I advance to explain you.

The problems must be between in a range and there mustn't be repeat problems.

I was using several jagged arrays. The code for me is awful because sometimes I don't even understand it.

For example, there are problems that using the same structure

NUMBER1 OPERATOR NUMBER 2 = RESULT

This is the pattern for it:

        int[][] arrayLimits1 = new int[][]
        {
            new int[] { 6, 10, 3, 10 }, new int[] { 6, 10, 3, 10 },
            new int[] { 6, 10, 3, 10 }, new int[] { 6, 10, 3, 10 },
            new int[] { 6, 10, 3, 10 }, new int[] { 6, 10, 3, 10 },
            new int[] { 6, 10, 3, 10 }, new int[] { 6, 10, 3, 10 },
            new int[] { 6, 10, 3, 10 }, new int[] { 6, 10, 3, 10 },
            new int[] { 15, 50, -95, -50 }, new int[] { -50, -15, -95, -50 }, new int[] { -50, -15, -95, -50 },
            new int[] { 15, 50, -95, -50 }, new int[] { -50, -15, -95, -50 }, new int[] { -50, -15, -95, -50 },
            new int[] { 6, 10, -10, -3 }, new int[] { -10, -6, 3, 10 }, new int[] { -10, -6, -10, -3 },
            new int[] { 6, 10, -10, -6, 1 }, new int[] { -10, -6, 6, 10, 1 }, new int[] { -10, -6, -10, -6, 1 }, 
            new int[] { 300, 900, 15, 90 }, new int[] { 300, 900, 25, 50 }, new int[] { 2000, 10000, 105, 900 }, new int[] { 12, 20, 25, 50, 1},
            new int[] { 10, 50, -50, -10 }, new int[] { -100, -50, -100, -50 }, new int[] { -10, 10, -10, 10 }, new int[] { -100, 0, -100, 0  }   
        };

        int[][] problems1 = GeneratePattern1(arrayLimits1);

The generate function receives the array. The first index in the array means min and max of NUMBER1, and others two for NUMBER2.

    private int[][] GeneratePattern1(int[][] arrayLimits)
    {
        int[][] sel = new int[arrayLimits.Length][];
        for (int i = 0; i < arrayLimits.Length; i++)
        {
            int[] temp;
            do
            {
                temp = Mathematics.GeneratePairOfIntegers(arrayLimits[i][0], arrayLimits[i][1], arrayLimits[i][2], arrayLimits[i][3]);

                if (arrayLimits[i].Length > 4)
                    temp[0] *= temp[1];

            } while (AuxiliaryMethods.Exists(sel, temp, i));

            sel[i] = temp;
        }

        return sel;
    }

    public static int[] GeneratePairOfIntegers(int li1, int ls1, int li2, int ls2)
    {
        int[] pair = new int[2];
        pair[0] = randomizer.Next(li1, ls1);
        pair[1] = randomizer.Next(li2, ls2);

        return pair;
    }

    public static bool Exists<T>(T[] array, T elem, int capacity)
    {
        for (int i = 0; i < capacity; i++)
        {
            if (array[i].Equals(elem))
                return true;
        }
        return false;
    }

All the array has Count=4, except some because has a number "1". This means that the problem is a division and I want to generate divisions with a remainder equals to zero.

I had to create another function for each type of problem. The last function was for integer problems, but if I want doubles:

    private double[][] GeneratePattern2(double[][] arrayLimits)
    {
        double[][] sel = new double[arrayLimits.Length][];
        for (int i = 0; i < arrayLimits.Length; i++)
        {
            double[] temp;
            do
            {
                temp = Mathematics.GeneratePairOfDoubles(arrayLimits[i][0], arrayLimits[i][4], (int)arrayLimits[i][5],
                                                         arrayLimits[i][6], arrayLimits[i][7], (int)arrayLimits[i][5]);

                if (arrayLimits[i].Length > 4)
                    temp[0] = Convert.ToDouble(Convert.ToDecimal(temp[1]) * Convert.ToDecimal(temp[0]));

            } while (AuxiliaryMethods.Exists(sel, temp, i));

            sel[i] = temp;
        }

        return sel;
    }

    public static double[] GeneratePairOfDoubles(double li1, double ls1, int decimals1, double li2, double ls2, int decimals2)
    {
        double[] pair = new double[2];
        pair[0] = randomizer.NextDouble(li1, ls1, decimals1);
        pair[1] = randomizer.NextDouble(li2, ls2, decimals2);

        return pair;
    }

I think I'm being redundant. I need to generate the problems. How can I generate a class to store each one? Because I'm using several jagged arrays and I think that's bad.

  • \$9 x 9\$ = ____ <-- Only an integer
  • \$49 + -89\$ = ____ <-- Only an integer
  • \$0.75\$ (decimal) = _ (fraction) <-- Two integers
  • \$\frac{3}{4}\$ (fraction) = _ (decimal) <-- A double
share|improve this question

closed as off-topic by Jamal Oct 18 at 20:48

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

6  
Great that you've come forward to improve yourself! All code seems messy and unfinished after a few years of becoming a different kind of a beginner. To help us help you, you could pick some key parts of the code you want to get review and paste it to this question - it's a lot friendlier than having to go through megaupload and whatever comes from there. If there are several different things you want to get reviewed then you might create a separate question for each of them. If you want your classes and their relationships to be reviewed then a simple class diagram may help. –  Aleksi Yrttiaho Aug 2 '11 at 6:35
5  
You say your problem is that you want to work uniformly with those problems, but they are different in details. So you have different classes of problems. Start by creating classes for types of problems you have and figure out what you want to do with those problems in general and create an interface or abstract class for that. –  Gabriel Ščerbák Aug 7 '11 at 0:22
5  
Gabriel is right about the interface/abstracts, identify the activities and data elements and design some contracts for starters. The other thing I wanted to say is, post smaller pieces of contiguous and specific code for review at a time. Often times cleaned up code makes available design improvements obvious, which I think is what you're more interested in anyways. –  Jimmy Hoffa Aug 10 '11 at 12:29
1  
Code is data and data is code. Use a Lisp. –  Leonid Apr 20 '12 at 22:11
    
+1 to @Leonid comment. While fundamental const, car, ctr structures of Lisp are unfamiliar to C#'ers, we too should build fundamental data/code structures (classes) that gives order, control, and understanding to our data; in this case messy Arrays of Arrays. Being fundamental we do this first. When I think of my classes as "data + its structure" (literally, I think this) much better code just flows out ... naturally. –  radarbob Sep 18 '13 at 15:29

2 Answers 2

  • The code for me is awful because sometimes I don't even understand it.
  • There are problems that using the same structure ( NUMBER1 OPERATOR NUMBER 2 = RESULT )
  • I had to create another function for each type of problem. Last function was for integer problems but if I want doubles..

Design Data Structures First

Up front, here is THE take-away: Most of the confusion, complexity, awful code, will simply resolve itself when we make a structure - a class - to hold the math problem stuff.

We need to be writing the code in terms of our problem space - operand, operators, limits, etc. NOT in terms of arrayLimits1[4][0]. We need structure that works for us.

  1. We need something to hold all the parts of a math problem. I expect we want all the parts in one object; it will just be easier to handle.
  2. We need to handle different numeric types.
  3. We need to represent the operator - when I need to define a limited set of things I look to enum

NOTE: I did not come up with (below) in one thought. But I did start with #1, the idea that we want a class for a MathProblem - which includes all that array crap. We are, after all, trying to do Object Oriented Design.

public enum Operator {
    unknown, add, subtract, multiply, divide
}

public class MathProblem<T> {
    // T is int, double, etc.

    public int Op1Max {get; private set;}
    public int Op1Min {get; private set;}
    public int Op2Max {get; private set;}
    public int Op2Min {get; private set;}

    public T Operand1 {get; private set;}
    public T Operand2 {get; private set;}
    public T Result {get; set;}

    public Operator {get; set;}

    public MathProblem (int[] operandLimits) {
        Op1Max = operandLimits[0];
        Op1Min = operandLimits[1];
        Op2Max = operandLimits[2];
        Op2Min = operandLimits[3];
    }
}

Think Big Picture

From the problem description It should be clear(ish) that we

  1. Get the limits in the object
  2. Create the operands based on the limits
  3. decide what operation to do
  4. Calculate the answer.

Hey!... Holy differential calculus, Batman! I think we just wrote the main program here.

public static void main() {
    BuildObjects();
    GenerateOperands();
    SetMathOperators();
    CalculateResults();
    PrintResults();
}

// write the method shells so the code will compile
public void BuildObjects(){}
public void GenerateOperands(){}
public void SetMathOperators(){}
public void CalculateResults(){}
public void PrintResults(){}

We'll change the above as needed when we figure out details.

Build Objects

public void BuildObject(int[][] operandRanges) {
    // Ah! just now I realize we need to decide what numeric type to make.
    // we'll do just one for now.
    List<MathProblem<int>> problemsOfInt = new List<MathProblem<int>>();

    for (int i=0; i< arrayLimits1.Length; i++) {
        problemsOfInt.Add( new MathProblem<int>(arrayLimits1[i]) );
    }
}

Wow! "brevity is the soul of good OO design".

******* go back to the main() right now and put the parameter in the call, so we don't get a compile error. In general, DO NOT KEEP CODING IF IT WON'T COMPILE

Generate Numbers

I have not thought about exactly how I'm going generate the operands (given the limits stashed in the objects), that's for you to solve.

I strongly suspect that "the code is awful" because you jumped right into trying to figure out how to do this stuff; never giving one thought data structure or even elementary program structure.

Look how simple the code is! This is a direct result of designing a data structure up front!

// Take the min/max limits in each object and generate operands.
// put the operands in the object.
public void GenerateOperands (List<MathProblem<int>> problems) {
    // Ah. looks like we're gonna need methods for each `MathProblem<T>`
    // and I'm sensing refactoring in our future.

    foreach (var problem in problems) {
       GenerateIntOperands (problem); 
    }
}

GenerateOperands

MathProblem structure again makes things easy to understand.

public void GenerateIntOperands(MathProblem<int> problem) {
    problem.Operand1 = randomizer (problem.Op1Max, problem.Op1Min);
    problem.Operand2 = randomizer (problem.Op2Max, problem.Op2Min);

   // looks like we need to make Operand setters "public"
}

Iterate List in each method, rinse, repeat

Ok, you get the idea.

Refactoring

I think I'm being redundant.

And I got the same feeling. Notice how we discovered that we needed a set of duplicate methods for each type - int, double, etc.

If we take all those methods and put them in a class with a type parameter, just like MathProblem<T> then we get rid of the redundancy.

public void ProblemProcessor<T> {
    // T is MathProblem<int>, MathProblem<double>, etc.

    protected List<T> MathProblems;
    protected int[][] OperandRanges;

    public ProblemProcessor (int[][] operandRanges) {
        MathProblems = new List<T>();
        OperandRanges = operandRanges;
    }

   // Just now I realized that good object orientation means the
   // client code should not have to know what methods to call, or in 
   // what order. ProblemProcessor should
   // know how to process problems. The client just must be able to
   // tell him to do it. Self responsibility is a hallmark of OO.

    public void Execute() {
        BuildObjects(OperandRanges);
        GenerateOperands(MathProblems);
        SetMathOperators();
        CalculateResults();
        PrintResults();
    }

    // With class variables we don't really need method parameters.
    // However they help understanding without digging into details.
    // The bigger/busier the class the more parameters help in this regard.
    // It's a judgement call.

    protected void BuildObjects(int[][] operandRanges) {}
    protected void GenerateOperands(List<t> problems) {} 
    protected void SetMathOperators() {}
    protected void CalculateResults() {}
    protected void PrintResults() {}
}

Finally!

public static void main () {
    ProblemProcessor<MathProblem<int>> intProblems = new ProblemProcessor<MathProblem<int>> (arrayLimits1);
    ProblemProcessor<MathProblem<double>> doubleProblems = new ProblemProcessor<MathProblem<double>> (arrayLimits1);
    ProblemProcessor<MathProblem<float>> floatProblems = new ProblemProcessor<MathProblem<float>> (arrayLimits1);

    intProblems.Execute();
    doubleProblems.Execute();
    floatProblems.Execute();
}

Arrays? We don't need no stinkin' Arrays!

share|improve this answer

Without attempting to rewrite the entire code, I think you need to think about keeping your functions with your data. For instance, let's say you have an IBounds interface.

interface IBounds {
  Tuple<double, double> GeneratePairsOfDoubles();
}

you can then declare implementations

class MultiplicationBounds : IBounds {}
class DivisionBounds : IBounds {}

Each of these "Bounds" objects would correspond to one of the int arrays, but would have named fields saying what each element did.

This is obviously an over-simplification of what you're trying to do, but it you break it down like this, you'll eventually find the class structure that works for your project. Even in dynamically typed languages, you don't keep behaviour too separated from the data.

share|improve this answer

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