gemmellr commented on code in PR #5487:
URL: https://github.com/apache/activemq-artemis/pull/5487#discussion_r1952359866
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/RandomUtil.java:
##########
@@ -31,17 +31,56 @@ public static Random getRandom() {
return random;
}
+ private static String letters = "abcdefghijklmnopqrstuvwxyz";
+
+ private static String digits = "0123456789";
+
+ private static String randomBase = letters + letters.toUpperCase() + digits;
+
+ /**
+ * Utility method to build a {@code String} filled with random
alpha-numeric characters. The {@code String} will
+ * contain characters from the following:
+ * <ul>
+ * <li>abcdefghijklmnopqrstuvwxyz</li>
+ * <li>ABCDEFGHIJKLMNOPQRSTUVWXYZ</li>
+ * <li>0123456789</li>
+ * </ul>
+ * @param length how long the returned {@code String} should be
+ * @return a {@code String} of random alpha-numeric characters
+ */
+ public static String randomAlphaNumericString(int length) {
+ StringBuilder result = new StringBuilder(length);
+ for (int i = 0; i < length; i++) {
+ result.append(randomBase.charAt(randomInterval(0,
randomBase.length())));
Review Comment:
So, looking at the impl of randomInterval (which I didn't when reviewing the
addition here before), I can see now that its actually not _exactly_ either of
the options I said, but rather a third [variant of the first] option.
It turns out randomInterval(..) _can_ pick the 'max', but _only_ if its
equal the 'min', i.e meaning there is actually no random possibility, but then
in the more typical use where the two args actually differ it can indeed
_never_ pick 'max' due to the behaviour of Random#nextInt which you quoted,
where the calculated singular 'bound' passed to that can never be returned.
So in this exact setup, with min and max essentially fixed at 0 and 62,
randomAlphaNumericString (..) will indeed work as expected...but only because
randomInterval(..) is arguably wrong in that it both can and cant return 'max'
depending on what you feed it. If it were ever changed such that the 'max'
could generally be returned (as it can in the other method RandomUtil has with
a 'max') then randomAlphaNumericString(..) would become broken and would
randomly throw.
I would simply change the implementation of randomAlphaNumericString to just
directly call _random.nextInt(randomBase.length())_ by itself instead of using
randomInterval(..) to do it, and we can consider whether to change that
separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
