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 guess I might use delegates. But I'm not certain if I can apply for it.

Sorry if the code is a mess. I'm a beginner and am still learning. For that reason, I need a little of help to improve this code. It's not finished.

I'm creating a Math quiz system. And there are many questions as you will be able to see in it, they're so different and I have a difficult moment to create the classes.

    private int[] Generate_Fraction(int li1, int ls1, int li2, int ls2)
    {
        int numerator = randomizer.Next(li1, ls1);
        int denominator = randomizer.Next(li2, ls2);

        int[] fraction = new int[2];
        fraction[0] = numerator;
        fraction[1] = denominator;

        return fraction;
    }

    // Decimals
    private double[] GeneratePattern5(double[][] limits)
    {
        double[] decimals = new double[limits.Length];
        for (int i = 0; i < limits.Length; i++)
        {
            bool flag;
            double decTemp;

            do
            {
                flag = false;
                decTemp = GetDoubleBetween(limits[i][0], limits[i][1], (int)limits[i][2]);

                if (Exists(decimals, decTemp, i))
                {
                    flag = true;
                }

            } while (flag);

            decimals[i] = decTemp;
        }

        return decimals;

    }

    // Fractions like: 6/60
    private int[][] GeneratePattern7(int[][] arrayLimits)
    {
        int[][] fractions = new int[arrayLimits.Length][];
        for (int i = 0; i < arrayLimits.Length; i++)
        {
            bool flag;
            int[] fracTemp = new int[2];
            do
            {
                flag = false;
                fracTemp = Generate_Fraction(arrayLimits[i][0], arrayLimits[i][1], arrayLimits[i][2], arrayLimits[i][3]);
                fracTemp[1] *= fracTemp[0];

                if (Exists(fractions, fracTemp, i))
                {
                    flag = true;
                }
            } while (flag);

            fractions[i] = fracTemp;
        }

        return fractions;
    }
share|improve this question

migrated from stackoverflow.com Aug 1 '11 at 5:11

This question came from our site for professional and enthusiast programmers.

3  
I think you mean generics, not delegates? –  Tim Jul 29 '11 at 21:13
1  
Can you be more specific, which part of the code do you think is verbose? –  Tony Jul 29 '11 at 21:14
    
The return types and methods of generating the values differ greatly, so I don't see any obvious way to do it. What do you think is wrong with the way you have it? –  ccurrens Jul 29 '11 at 21:16
    
Well, that's an example code from my solution. I've got to generate many patterns, if you see the subroutines GeneratePattern5 and GeneratePattern7, they're doing the same thing, only verify if the data exists in the list. Do you get me? –  oscar.fimbres Jul 29 '11 at 21:19
    
Well, 7 is a lucky number. How the number of toes on your foot figures into this is pretty unguessable. –  Hans Passant Jul 29 '11 at 21:23

2 Answers 2

Unless I'm missing something, you can move the Exists inside the condition and remove the flag variable. This will change the loop to...

        double decTemp;

        do
        {
            decTemp = GetDoubleBetween(limits[i][0], limits[i][1], (int)limits[i][2]);
        } while (Exists(decimals, decTemp, i));

I believe that this is more semantically in line with what you are trying to accomplish. You could do the same thing inside the GeneratePattern7 method as well. Doing so will reduce a lot of clutter.

share|improve this answer

You can remove duplication from this code. There are a couple of ways to do this but as a result you'll get really complicated code. I think you should leave it as it is now. There are situations when it's better to have some duplication than to have complicated code.

share|improve this answer

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.