-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
Replace generic dictionary for commands with specialized collection type. #203
Changes from 3 commits
c84f4ba
9cb3584
c9a41b7
eac254b
ad7324c
5ea9f3a
ff5e80b
0bceaf3
ec8ecee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,101 @@ | ||||||||||||||||||||||||||||
// Copyright (c) Six Labors. | ||||||||||||||||||||||||||||
// Licensed under the Apache License, Version 2.0. | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
using System; | ||||||||||||||||||||||||||||
using System.Collections.Generic; | ||||||||||||||||||||||||||||
using System.Collections.ObjectModel; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
namespace SixLabors.ImageSharp.Web.Commands | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
/// <summary> | ||||||||||||||||||||||||||||
/// Represents an ordered collection of processing commands. | ||||||||||||||||||||||||||||
/// </summary> | ||||||||||||||||||||||||||||
public sealed class CommandCollection : KeyedCollection<string, KeyValuePair<string, string>> | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
private readonly List<string> keys = new(); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// <summary> | ||||||||||||||||||||||||||||
/// Initializes a new instance of the <see cref="CommandCollection"/> class. | ||||||||||||||||||||||||||||
/// </summary> | ||||||||||||||||||||||||||||
public CommandCollection() | ||||||||||||||||||||||||||||
: this(StringComparer.OrdinalIgnoreCase) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
private CommandCollection(IEqualityComparer<string> comparer) | ||||||||||||||||||||||||||||
: base(comparer) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// <summary> | ||||||||||||||||||||||||||||
/// Gets an <see cref="ICollection{T}"/> representing the keys of the collection. | ||||||||||||||||||||||||||||
/// </summary> | ||||||||||||||||||||||||||||
public ICollection<string> Keys => this.keys; | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also use the internal lookup dictionary (which might be null, which might need a fallback to an empty list):
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The internal lookup There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, of course, I already thought I was missing something! 😖 I see this is only used in
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a neat suggestion! The enumerator should still be fast here. I'll not commit this directly as it means I can remove a bunch of overrides at the same time. |
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// <summary> | ||||||||||||||||||||||||||||
/// Gets or sets the value associated with the specified key. | ||||||||||||||||||||||||||||
/// </summary> | ||||||||||||||||||||||||||||
/// <param name="key">The key of the value to get or set.</param> | ||||||||||||||||||||||||||||
/// <returns> | ||||||||||||||||||||||||||||
/// The value associated with the specified key. If the specified key is not found, | ||||||||||||||||||||||||||||
/// a get operation throws a <see cref="KeyNotFoundException"/>, and | ||||||||||||||||||||||||||||
/// a set operation creates a new element with the specified key. | ||||||||||||||||||||||||||||
/// </returns> | ||||||||||||||||||||||||||||
/// <exception cref="ArgumentNullException"><paramref name="key"/> is null.</exception> | ||||||||||||||||||||||||||||
/// <exception cref="KeyNotFoundException">An element with the specified key does not exist in the collection.</exception> | ||||||||||||||||||||||||||||
public new string this[string key] | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
get | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
if (this.TryGetValue(key, out KeyValuePair<string, string> item)) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
return item.Value; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
throw new KeyNotFoundException(); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
set | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
if (this.TryGetValue(key, out KeyValuePair<string, string> item)) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
this.SetItem(this.IndexOf(item), new(key, value)); | ||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
throw new KeyNotFoundException(); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// <inheritdoc/> | ||||||||||||||||||||||||||||
protected override void InsertItem(int index, KeyValuePair<string, string> item) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
base.InsertItem(index, item); | ||||||||||||||||||||||||||||
this.keys.Insert(index, item.Key); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// <inheritdoc/> | ||||||||||||||||||||||||||||
protected override void RemoveItem(int index) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
base.RemoveItem(index); | ||||||||||||||||||||||||||||
this.keys.RemoveAt(index); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// <inheritdoc/> | ||||||||||||||||||||||||||||
protected override void SetItem(int index, KeyValuePair<string, string> item) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
base.SetItem(index, item); | ||||||||||||||||||||||||||||
this.keys[index] = item.Key; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// <inheritdoc/> | ||||||||||||||||||||||||||||
protected override void ClearItems() | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
base.ClearItems(); | ||||||||||||||||||||||||||||
this.keys.Clear(); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// <inheritdoc/> | ||||||||||||||||||||||||||||
protected override string GetKeyForItem(KeyValuePair<string, string> item) => item.Key; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// Copyright (c) Six Labors. | ||
// Licensed under the Apache License, Version 2.0. | ||
|
||
using System.Collections.Generic; | ||
|
||
namespace SixLabors.ImageSharp.Web.Commands | ||
{ | ||
/// <summary> | ||
/// Extension methods for <see cref="CommandCollectionExtensions"/>. | ||
/// </summary> | ||
public static class CommandCollectionExtensions | ||
{ | ||
/// <summary> | ||
/// Gets the value associated with the specified key or the default value. | ||
/// </summary> | ||
/// <param name="collection">The collection instance.</param> | ||
/// <param name="key">The key of the value to get.</param> | ||
/// <returns>The value associated with the specified key or the default value.</returns> | ||
public static string GetValueOrDefault(this CommandCollection collection, string key) | ||
deanmarcussen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
collection.TryGetValue(key, out KeyValuePair<string, string> result); | ||
return result.Value; | ||
} | ||
} | ||
} |
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.
Keep in mind that this constructor already creates an internal lookup dictionary: https://docs.microsoft.com/en-us/dotnet/api/system.collections.objectmodel.keyedcollection-2.-ctor?view=net-6.0#system-collections-objectmodel-keyedcollection-2-ctor(system-collections-generic-iequalitycomparer((-0))-system-int32).
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 considered implementing
IDictionary<string, string>
but it leads to a confusing API since the type inheritsCollection<T>
.Contains
vsContainsKey
for example, I also didn't want to expose some unordered properties likeValues
. There's also virtualization issues passing aroundIDictionary<T>
all the time. I also experimented with using a wrapper type with a private implementation ofKeyedCollection<T,TItem>
but this started to get messy quickly so I took the easy route.I'm not quite sure what you're pointing out re the constructor. Are you suggesting using the internal dictionary to implement the interface or for something else?
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.
Confusion can be avoided by using an explicit interface implementation (which might already be required for some conflicting members)...
The point I wanted to make, is that this implementation stores the items both in the collection and internal lookup dictionary, in addition to keeping a sorted list of keys.
Too bad
OrderedDictionary
isn't using generics, although I see there is an internal MS implementation.