-
-
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
Merged
Merged
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c84f4ba
Replace generic dictionary with specialized collection type.
JimBobSquarePants 9cb3584
Add tests
JimBobSquarePants c9a41b7
Allow set via key
JimBobSquarePants eac254b
Update CommandCollectionTests.cs
JimBobSquarePants ad7324c
Use IEnumerable Keys and fix indexer
JimBobSquarePants 5ea9f3a
Remove need to allocate new list to find the index
JimBobSquarePants ff5e80b
Add convenience methods.
JimBobSquarePants 0bceaf3
Remove DictionaryExtensions
JimBobSquarePants ec8ecee
Add comments
JimBobSquarePants File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
// Copyright (c) Six Labors. | ||
// Licensed under the Apache License, Version 2.0. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Collections.ObjectModel; | ||
using System.Runtime.CompilerServices; | ||
|
||
namespace SixLabors.ImageSharp.Web.Commands | ||
{ | ||
/// <summary> | ||
/// Represents an ordered collection of processing commands. | ||
/// </summary> | ||
public sealed class CommandCollection : KeyedCollection<string, KeyValuePair<string, string>> | ||
{ | ||
/// <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="IEnumerable{String}"/> representing the keys of the collection. | ||
/// </summary> | ||
public IEnumerable<string> Keys | ||
{ | ||
get | ||
{ | ||
foreach (KeyValuePair<string, string> item in this) | ||
{ | ||
yield return this.GetKeyForItem(item); | ||
} | ||
} | ||
} | ||
|
||
/// <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; | ||
} | ||
|
||
ThrowKeyNotFound(); | ||
return default; | ||
} | ||
|
||
set | ||
{ | ||
if (this.TryGetValue(key, out KeyValuePair<string, string> item)) | ||
{ | ||
this.SetItem(this.IndexOf(item), new(key, value)); | ||
} | ||
else | ||
{ | ||
this.Add(new(key, value)); | ||
} | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Searches for an element that matches the conditions defined by the specified | ||
/// predicate, and returns the zero-based index of the first occurrence within the | ||
/// entire <see cref="CommandCollection"/>. | ||
/// </summary> | ||
/// <param name="match"> | ||
/// The <see cref="Predicate{T}"/> delegate that defines the conditions of the element to | ||
/// search for. | ||
/// </param> | ||
/// <returns> | ||
/// The zero-based index of the first occurrence of an element that matches the conditions | ||
/// defined by match, if found; otherwise, -1. | ||
/// </returns> | ||
/// <exception cref="ArgumentNullException"><paramref name="match"/> is null.</exception> | ||
public int FindIndex(Predicate<string> match) | ||
{ | ||
Guard.NotNull(match, nameof(match)); | ||
|
||
int index = 0; | ||
foreach (KeyValuePair<string, string> item in this) | ||
{ | ||
if (match(item.Key)) | ||
{ | ||
return index; | ||
} | ||
|
||
index++; | ||
} | ||
|
||
return -1; | ||
} | ||
|
||
/// <inheritdoc/> | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
protected override string GetKeyForItem(KeyValuePair<string, string> item) => item.Key; | ||
|
||
[MethodImpl(MethodImplOptions.NoInlining)] | ||
private static void ThrowKeyNotFound() => throw new KeyNotFoundException(); | ||
} | ||
} |
25 changes: 25 additions & 0 deletions
25
src/ImageSharp.Web/Commands/CommandCollectionExtensions.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.