Skip to content

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

Merged
merged 2 commits into from
Dec 10, 2017

Conversation

bahusoid
Copy link
Member

@bahusoid bahusoid commented Dec 8, 2017

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.

@@ -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>()
Copy link
Member

@hazzik hazzik Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to EmptyMap or EmptyDictionary

Copy link
Member Author

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.

@bahusoid
Copy link
Member Author

bahusoid commented Dec 8, 2017

I didn't touch Test project. Let me know if it also needs to be updated (for consistency or whatever reasons)

Copy link
Member

@fredericDelaporte fredericDelaporte left a 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;
Copy link
Member

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>();
Copy link
Member

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>();

...

@bahusoid
Copy link
Member Author

bahusoid commented Dec 9, 2017

@fredericDelaporte Ok. Did as you suggested

@fredericDelaporte fredericDelaporte self-assigned this Dec 9, 2017
@fredericDelaporte fredericDelaporte added this to the 5.1 milestone Dec 10, 2017
@fredericDelaporte fredericDelaporte merged commit c4eb6f4 into nhibernate:master Dec 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants