-
Notifications
You must be signed in to change notification settings - Fork 934
Reuse the same generic EmptyMapClass instance across the project #1477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -214,6 +214,12 @@ public IEnumerator GetEnumerator() | |||
|
|||
public static readonly IEnumerable EmptyEnumerable = new EmptyEnumerableClass(); | |||
public static readonly IDictionary EmptyMap = new EmptyMapClass(); | |||
|
|||
public static IDictionary<TKey, TValue> EmptyMapFor<TKey, TValue>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to EmptyMap
orEmptyDictionary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hazzik Done. I've also moved Instance to separate class - EmptyDictionaryHolder<TKey, TValue>
. As EmptyMapClass<TKey, TValue>
is public class and it's better to avoid unnecessary static initialization if it wasn't requested explicitly.
765306f
to
cf041df
Compare
cf041df
to
61664a4
Compare
I didn't touch Test project. Let me know if it also needs to be updated (for consistency or whatever reasons) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't touch Test project. Let me know if it also needs to be updated (for consistency or whatever reasons)
Yes better update it too for consistency. Especially if doing my proposed changes, of course! (Release build would fail otherwise.)
|
||
public static IDictionary<TKey, TValue> EmptyDictionary<TKey, TValue>() | ||
{ | ||
return EmptyDictionaryHolder<TKey, TValue>.Instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather have it directly on EmptyMapClass
. So here it would be
return EmptyMapClass<TKey, TValue>.Instance;
@@ -457,6 +463,11 @@ public object Current | |||
#endregion | |||
} | |||
|
|||
private static class EmptyDictionaryHolder<TKey, TValue> | |||
{ | |||
public static readonly IDictionary<TKey, TValue> Instance = new EmptyMapClass<TKey, TValue>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this into EmptyMapClass
but changed to internal
.
And then I would introduced an obsolete default public constructor for avoiding future code re-instanciating it directly.
So EmptyMapClass
would start this way:
/// <summary>
/// A read-only dictionary that is always empty and permits lookup by <see langword="null" /> key.
/// </summary>
[Serializable]
public class EmptyMapClass<TKey, TValue> : IDictionary<TKey, TValue>
{
#pragma warning disable 618 // Constructor is obsolete, to be switched to non-obsolete but private.
internal static readonly IDictionary<TKey, TValue> Instance = new EmptyMapClass<TKey, TValue>();
#pragma warning restore 618
// Since v5.1. To be switched to private.
[Obsolete("Please use CollectionHelper.EmptyDictionary<TKey, TValue>() instead.")]
public EmptyMapClass() {}
private static readonly EmptyEnumerator<TKey, TValue> emptyEnumerator = new EmptyEnumerator<TKey, TValue>();
...
@fredericDelaporte Ok. Did as you suggested |
This is simple "find and replace" kind of pull request to eliminate not needed instantiating of immutable EmptyMapClass instances.
Added
CollectionHelper.EmptyDictionary<TKey, TValue>()
helper mehtod that returns the same instance for given TKey and TValue and use it across the project instead of creating new instances.