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;
}
-
\$\begingroup\$ The code doesn't do what you claim. \$\endgroup\$200_success– 200_success2014年11月16日 05:40:16 +00:00Commented Nov 16, 2014 at 5:40
-
\$\begingroup\$ @200_success i correct the code to produce the right values \$\endgroup\$lostForAWHile– lostForAWHile2014年11月23日 05:42:36 +00:00Commented Nov 23, 2014 at 5:42
2 Answers 2
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);
-
\$\begingroup\$ I agree this is an excellent point. Some more details would be very good \$\endgroup\$lostForAWHile– lostForAWHile2014年11月16日 04:18:32 +00:00Commented Nov 16, 2014 at 4:18
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;
}
-
\$\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\$lostForAWHile– lostForAWHile2014年11月16日 02:54:07 +00:00Commented Nov 16, 2014 at 2:54