Fast React ID Generator#528
Conversation
Daniel15
left a comment
There was a problem hiding this comment.
This seems OK to me. @dustinsoftware what do you think?
| var id = Interlocked.Increment(ref _random); | ||
|
|
||
| //reactPrefix.Length == 6 | ||
| chars[6] = _encode32Chars[(int)(id >> 60) & 31]; |
There was a problem hiding this comment.
There's a lot of magic numbers in this code. It'd be good to pull these out as constants and add comments explaining what they're doing.
| /// <summary> | ||
| /// Extension methods relating to GUIDs. | ||
| /// </summary> | ||
| public static class ReactIdGenerator |
There was a problem hiding this comment.
Would be good to add an interface for this, and use the dependency injection framework to inject it into ReactComponent. That way, anyone can swap out the implementation if they want to (eg. if they want the old one back).
There was a problem hiding this comment.
(see AssemblyRegistration.cs, use .AsSingleton())
There was a problem hiding this comment.
Nice idea, i can inject it to ReactEnvironment and create property IReactIdGenerator (like IBabel) and call it from constructor in ReactComponent like "environment.ReactIdGenerator.Generate(...)"
| var chars = _chars; | ||
| if (chars == null) | ||
| { | ||
| _chars = chars = new char[19]; |
| chars[17] = _encode32Chars[(int)(id >> 5) & 31]; | ||
| chars[18] = _encode32Chars[(int)id & 31]; | ||
|
|
||
| return new string(chars, 0, 19); |
There was a problem hiding this comment.
ok!, const will be inlined without overhead 👍
|
You can pass it in the constructor for ReactComponent rather than adding a
property to ReactEnvironment
Sent from my phone.
…On Mon, Apr 9, 2018, 7:13 AM Daniil Sokolyuk ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/React.Core/ReactIdGenerator.cs
<#528 (comment)>:
> + * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+using System;
+using System.Threading;
+
+namespace React
+{
+ /// <summary>
+ /// Extension methods relating to GUIDs.
+ /// </summary>
+ public static class ReactIdGenerator
Nice idea, i can inject it to IReactEnvironment and create property
IReactIdGenerator (like IBabel) and call it from constructor like
"environment.ReactIdGenerator.Generate(...)"
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#528 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFnHbQsP0g0FVqr52UqGGL4msV_r2fsks5tm2yjgaJpZM4TLl08>
.
|
|
Completed. |
dustinsoftware
left a comment
There was a problem hiding this comment.
Thanks! Looks good.
| namespace React | ||
| { | ||
| public class GuidExtensionsTests | ||
| public interface IReactIdGenerator |
There was a problem hiding this comment.
Ugh, sorry. Missed this one. There is no XML comment for this type which is generating a warning at build time.
Fast React ID Generator