NEO-59: address code review follow-ups
Simplify GetDefinitionsInIdOrder to iterate key-value pairs directly, avoiding the intermediate sorted-keys array and second dictionary lookup.pull/94/head
parent
dc66b5cef4
commit
1e1ca5519c
|
|
@ -33,16 +33,16 @@ None.
|
|||
|
||||
## Nits
|
||||
|
||||
- `GetDefinitionsInIdOrder()` iterates `.Keys` to build a sorted array then does a second dictionary lookup per key. Iterating key-value pairs directly is slightly more idiomatic and avoids the double lookup:
|
||||
- ~~`GetDefinitionsInIdOrder()` iterates `.Keys` to build a sorted array then does a second dictionary lookup per key. Iterating key-value pairs directly is slightly more idiomatic and avoids the double lookup:~~
|
||||
|
||||
```csharp
|
||||
return catalog.NodesById
|
||||
.OrderBy(kv => kv.Key, StringComparer.Ordinal)
|
||||
.Select(kv => kv.Value)
|
||||
.ToList();
|
||||
```
|
||||
~~```csharp~~
|
||||
~~return catalog.NodesById~~
|
||||
~~ .OrderBy(kv => kv.Key, StringComparer.Ordinal)~~
|
||||
~~ .Select(kv => kv.Value)~~
|
||||
~~ .ToList();~~
|
||||
~~```~~
|
||||
|
||||
The current form matches `ItemDefinitionRegistry` exactly, so consistency may outweigh the change. No action required.
|
||||
~~The current form matches `ItemDefinitionRegistry` exactly, so consistency may outweigh the change. No action required.~~ Done.
|
||||
|
||||
- The null-id and unknown-id tests each build a fully populated `nodes`/`yields` dictionary even though the exercised code paths exit before those entries are touched. Consistent with `ItemDefinitionRegistryTests` — no action required.
|
||||
|
||||
|
|
|
|||
|
|
@ -44,15 +44,9 @@ public sealed class ResourceNodeDefinitionRegistry(ResourceNodeCatalog catalog)
|
|||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public IReadOnlyList<ResourceNodeDefRow> GetDefinitionsInIdOrder()
|
||||
{
|
||||
var ids = catalog.NodesById.Keys.OrderBy(k => k, StringComparer.Ordinal).ToArray();
|
||||
var list = new List<ResourceNodeDefRow>(ids.Length);
|
||||
foreach (var id in ids)
|
||||
{
|
||||
list.Add(catalog.NodesById[id]);
|
||||
}
|
||||
|
||||
return list;
|
||||
}
|
||||
public IReadOnlyList<ResourceNodeDefRow> GetDefinitionsInIdOrder() =>
|
||||
catalog.NodesById
|
||||
.OrderBy(kv => kv.Key, StringComparer.Ordinal)
|
||||
.Select(kv => kv.Value)
|
||||
.ToList();
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue