Fix Random usages in utility classes

[SeppPenner]

Fixes https://github.com/mariusmuntean/ChartJs.Blazor/projects/3#card-28659459.

[Joelius300]

Actually while you're at it, would you like to fix the Random usage in PointUtil as well? You would rename it to s_rand there too and put a lock around the two NextDouble statements in GetRandomPoint. You could rename this PR to Fix Random usages in utility classes and kill two birds with one stone.

You don't have to, just an idea :)

[SeppPenner]

You would rename it to s_rand there too and put a lock around the two NextDouble statements in GetRandomPoint.

Do you mean to put the lock around the two statements or just around the calls to s_rand?

Like this?

lock(s_rand)
{
    var x = minX + (s_rand.NextDouble() * (maxX - minX));
    var y = minY + (s_rand.NextDouble() * (maxY - minY));
}

Or like this?

var x = minX + (lock(s_rand){s_rand.NextDouble()} * (maxX - minX));
var y = minY + (lock(s_rand){s_rand.NextDouble()} * (maxY - minY));

[Joelius300]

A single lock around the two NextDouble. You'll have to move the two variables x and y outside the lock of course.

Like this:

double x;
double y;
lock (s_rand)
{
    x = minX + (s_rand.NextDouble() * (maxX - minX));
    y = minY + (s_rand.NextDouble() * (maxY - minY));
}

[SeppPenner]

A single lock around the two NextDouble. You'll have to move the two variables x and y outside the lock of course.

I already assumed that :) Ok, changes will be there soon.

But the variables need to be of type double, I would say:

double x;
double y;

lock (s_rand)
{
    x = minX + (s_rand.NextDouble() * (maxX - minX));
    y = minY + (s_rand.NextDouble() * (maxY - minY));
}

[Joelius300]

But the variables need to be of type double, I would say

Of course, my bad. I edited my comment but I was a little bit too late I guess :)

Looks good but I have a nitpick. We're not using ColorString inside RandomColorString for whatever reason. When trying to figure out why not, I realized it's because of byte conversion. So here's how I would implement it now. I don't have write access to the fork so I'll post it here like this. The improved method is faster and doesn't use the hardcoded string again but instead delegates it to ColorString to get the same result (I tested it). If you can replace the RandomColorString method body with this, I'll merge it :)

byte[] rgb = new byte[3];
double alpha;

lock (s_rand)
{
    s_rand.NextBytes(rgb);
    alpha = s_rand.NextDouble();
}

return ColorString(rgb[0], rgb[1], rgb[2], alpha);

[SeppPenner]

It's changed now :)

[Joelius300]

Just an FYI

I wanted to back up by claim that my method was faster and did some extensive benchmarks. It's actually a little bit slower. The difference is almost invisible (~5 ms on 5 million iterations/2.6s). However it does consume a bit less memory (~30mb on 5 million interations/1.3gb).

I don't really know what's better but I came back to this because I thought allocating an array each time might be bad so I tested it. The best option would be to use stackalloc and Span<byte> which seems to allocates less memory (~200mb on 5 million iterations/1.3gb) and is pretty much the same speed as the original method (even less of a difference than the array one).

This is extreme microoptimization and we really shouldn't focus on that but I was curious so there you go.

Maybe I'll put it in but only to satisfy myself, it doesn't do anything for the library except force the dependency on .netstandard 2.1 because of span 😅

Span<byte> rgb = stackalloc byte[3];
double alpha;

lock (s_rand)
{
    s_rand.NextBytes(rgb);
    alpha = s_rand.NextDouble();
}

return ColorString(rgb[0], rgb[1], rgb[2], alpha);

[SeppPenner]

Haha, ok :) Yes, that's micro optimization, but if it uses less memory, why not 😄