2
\$\begingroup\$

I have a function that produce an array with values that are Gaussian (normal) distribution. If you plot the values on a graph, it would look something like this:

Gussian

Is there any way to improve on the code?

private static IComparable[] NonUniformDistributionsGaussian(int startNumber,int arraySize)
    {
        IComparable [] arr = new IComparable[arraySize];
        Double[] intervals = { ((1.00 / 6.00) * arraySize), ((2.00 / 6.00) * arraySize),
                                 ((3.00 / 6.00) * arraySize),((4.00 / 6.00) * arraySize),
                                 ((5.00 / 6.00) * arraySize), ((6.00 / 6.00) * arraySize) };

        for (var i = 0; i < arraySize; i++)
        {

            if (i <= (int)intervals[0])
            {
                startNumber = startNumber + 1;                   
            }

            else if ( i <= (int)intervals[1])
            {
                 startNumber = startNumber  + 2;                    
            }
            else if (i <= (int)intervals[2])
            {
                startNumber = startNumber + 3;
            }
            else if (i <= (int)intervals[3])
            {
                startNumber = startNumber - 3;
            }
            else if (i <= (int)intervals[4])
            {
                startNumber = startNumber - 3;
            }
            else 
            {
                 startNumber = startNumber  - 2;                     
            }
            arr[i] = startNumber;
        }
        return arr;
    }
\$\endgroup\$
  • \$\begingroup\$ The code doesn't do what you claim. \$\endgroup\$ – 200_success Nov 16 '14 at 5:40
  • \$\begingroup\$ @200_success i correct the code to produce the right values \$\endgroup\$ – DacorieS Nov 23 '14 at 5:42
4
\$\begingroup\$

As the conditions of the if statements only depending on some fixed numbers and the arraysize input parameter these values can be precalculated.

int oneThirdArraySize = (int)((1.00 / 3.00) * arraySize);

and then used in the loop like

for (var i = 0; i < arraySize; i++)
{

    if (i <= oneThirdArraySize)
    {
        startNumber = startNumber + 1;
        arr[i] = startNumber;
    }
    // and so on
}

By precalculation of the right side values of the if conditions outside of the loop you can speed this up because right now you do these calculation up to two times for every iteration.

But we can do better because you are using some magic numbers here which we can hide behind some meaningful const variables.

private static const double oneThird = 1d / 3d;
private static const double twoThird = 2d / 3d;  

In this way the calculations need to be changed to

int oneThirdArraySize = (int)(oneThird * arraySize);
\$\endgroup\$
  • \$\begingroup\$ I agree this is an excellent point. Some more details would be very good \$\endgroup\$ – DacorieS Nov 16 '14 at 4:18
3
\$\begingroup\$

I don't understand the logic or how it generates a Gaussian distribution.

Assuming the logic itself is correct, you could simplify the code.

Key points:

  • assignment statement identical in all if branches, so pull it out of the conditional logic (DRY principle)
  • No reason to do floating-point arithmetic - array indexes have to be integers so it's not clear what you're accomplishing
  • use if/else to avoid repeating the inverse of the first if branch (DRY principle again)
  • since there are only 3 mutually exclusive possibilities of the array index (first third, second third, final third) you don't even need to specify the final condition explicitly.

Improved version:

private static IComparable[] NonUniformDistributionsGaussian(int startNumber, int arraySize)
{
    IComparable [] arr = new IComparable[arraySize];
    for (var i = 0; i < arraySize; i++)
    {

        if (i <= arraySize / 3)
        {
            startNumber = startNumber + 1;
        }
        else if (i <= 2*arraySize/3)
        {
             startNumber = startNumber  + 2; 
        }
        else
        {
             startNumber = startNumber  - 2; 
        }
        arr[i] = startNumber;
    }
    return arr;
}
\$\endgroup\$
  • \$\begingroup\$ why i say it is a Gaussian distribution function is that the values in the array when plot on graph will look similar to the graph in my question. \$\endgroup\$ – DacorieS Nov 16 '14 at 2:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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