Skip to content

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.

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;


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