Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the project to target multiple frameworks, configures package metadata, and introduces new random generation utilities such as GUID, hex color, and console color generators. The review feedback highlights several critical areas for improvement, including resolving thread-safety issues with System.Random in multi-threaded environments, preventing integer and long overflows during range calculations, adding validation to ensure min <= max, fixing IRandomAlphabet to avoid returning non-letter characters, and caching the console colors array to eliminate redundant allocations.
| if (list == null || list.Count == 0) | ||
| throw new ArgumentException("List cannot be null or empty"); | ||
| return list[_random.Next(list.Count)]; | ||
| private static readonly System.Random _random = new System.Random(); |
There was a problem hiding this comment.
System.Random is not thread-safe. Concurrent access from multiple threads can cause it to enter a state where it always returns 0. Since ILibRandom is a static utility class, it is highly likely to be used in multi-threaded contexts. We can use conditional compilation to use System.Random.Shared on .NET 6+ and [ThreadStatic] on older frameworks.
#if NET6_0_OR_GREATER
private static System.Random _random => System.Random.Shared;
#else
[ThreadStatic]
private static System.Random? _localRandom;
private static System.Random _random => _localRandom ??= new System.Random();
#endif| public static int IRandomInt(int min, int max) | ||
| { | ||
| return _random.Next(min, max + 1); | ||
| } |
There was a problem hiding this comment.
If max is int.MaxValue, max + 1 overflows to int.MinValue, causing an ArgumentOutOfRangeException. Also, there is no validation that min <= max. We can resolve both issues by validating the input and using double arithmetic to calculate the range and offset safely.
public static int IRandomInt(int min, int max)
{
if (min > max)
throw new ArgumentException("min must be <= max");
double range = (double)max - (double)min;
int offset = (int)(_random.NextDouble() * (range + 1.0));
return min + offset;
}| public static long IRandomLong(long min, long max) | ||
| { | ||
| if (min > max) throw new ArgumentException("min must be <= max"); | ||
| long range = max - min; | ||
| long rand = (long)(_random.NextDouble() * (range + 1)); | ||
| return min + rand; | ||
| } |
There was a problem hiding this comment.
max - min can overflow long if min is negative and max is positive. Additionally, _random.NextDouble() only has 53 bits of precision, which leads to non-uniform distribution and skipped values for ranges larger than 2^53. We can use ulong to safely compute the range without overflow, and use NextBytes to get a full 64-bit random value.
public static long IRandomLong(long min, long max)
{
if (min > max) throw new ArgumentException("min must be <= max");
ulong range = (ulong)(max - min);
if (range == ulong.MaxValue)
{
byte[] buf = new byte[8];
_random.NextBytes(buf);
return BitConverter.ToInt64(buf, 0);
}
byte[] bytes = new byte[8];
_random.NextBytes(bytes);
ulong uval = BitConverter.ToUInt64(bytes, 0);
return min + (long)(uval % (range + 1));
}| public static char IRandomChar(char min, char max) | ||
| { | ||
| return (char)_random.Next(min, max + 1); | ||
| } |
There was a problem hiding this comment.
If min > max, the method will throw an ArgumentOutOfRangeException from _random.Next. Adding a guard clause to validate that min <= max ensures consistency with other methods and provides a clearer error message.
public static char IRandomChar(char min, char max)
{
if (min > max)
throw new ArgumentException("min must be <= max");
return (char)_random.Next(min, max + 1);
}| public static char IRandomAlphabet(char min, char max) | ||
| { | ||
| if (!char.IsLetter(min) || !char.IsLetter(max)) | ||
| throw new ArgumentException("Only alphabet characters allowed"); | ||
|
|
||
| return (char)_random.Next(min, max + 1); | ||
| } |
There was a problem hiding this comment.
If the range between min and max spans across the gap between uppercase and lowercase letters (e.g., IRandomAlphabet('A', 'z')), the method can return non-letter characters like [, \, ], ^, _, or ` because it simply selects a random character in the range without verifying if the generated character is actually a letter. We should use rejection sampling to ensure only letters are returned, and add a validation check for min > max.
public static char IRandomAlphabet(char min, char max)
{
if (min > max)
throw new ArgumentException("min must be <= max");
if (!char.IsLetter(min) || !char.IsLetter(max))
throw new ArgumentException("Only alphabet characters allowed");
char result;
do
{
result = (char)_random.Next(min, max + 1);
} while (!char.IsLetter(result));
return result;
}| public static string IRandomConsoleColor() | ||
| { | ||
| string[] colors = { "black", "darkblue", "darkgreen", "darkcyan", "darkred", | ||
| "darkmagenta", "darkyellow", "gray", "grey", "darkgray", | ||
| "darkgrey", "blue", "green", "cyan", "red", "magenta", | ||
| "yellow", "white" }; | ||
| return IRandomFromArray(colors); | ||
| } |
There was a problem hiding this comment.
The colors array is allocated on every call to IRandomConsoleColor(). Moving it to a private static readonly field avoids unnecessary allocations and improves performance.
private static readonly string[] ConsoleColors = {
"black", "darkblue", "darkgreen", "darkcyan", "darkred",
"darkmagenta", "darkyellow", "gray", "grey", "darkgray",
"darkgrey", "blue", "green", "cyan", "red", "magenta",
"yellow", "white"
};
public static string IRandomConsoleColor()
{
return IRandomFromArray(ConsoleColors);
}
No description provided.