Return IEnumerable<KeyValuePair> from a private method; use Dictionary or anon. type?

Question

Is it okay to return IEnumerable<KeyValuePair<string, string>> from a private method instead of returning a read-only dictionary? This allows for shorter/simpler code.

Note: In the below code, PropertyDictionary is a static property that returns an IReadOnlyDictionary<string, PropertyInfo> that was created through reflecting over the MyClass class looking for public, non-static, string-returning properties bearing a particular attribute. Also, the instance method that calls GetPropertyValues does some formatting, immediately changing the result into an IEnumerable<string>, so the dictionary aspect isn't needed for long.

private static IEnumerable<KeyValuePair<string, string>> GetPropertyValues(MyClass myClass) {
 return
 PropertyDictionary.ToDictionary(
 kvp => kvp.Key,
 kvp => (string) kvp.Value.GetValue(myClass, null)
 )
 .Where(kvp => !string.IsNullOrEmpty(kvp.Value));
}

vs.

private static IReadOnlyDictionary<string, string> GetPropertyValues(MyClass myClass) {
 return
 new ReadOnlyDictionary(
 PropertyDictionary.ToDictionary(
 kvp => kvp.Key,
 kvp => (string) kvp.Value.GetValue(myClass, null)
 )
 .Where(kvp => !string.IsNullOrEmpty(kvp.Value))
 .ToDictionary(kvp => kvp.Key, kvp => kvp.Value)
 );
}

This converts to a dictionary twice. I wish there were a way to do Where and Select at the same time! Perhaps there is a better way. Can you suggest one?

Also, should I use an anonymous type instead of a dictionary for the intermediate step?

private static IEnumerable<KeyValuePair<string, string>> GetPropertyValues(MyClass myClass) {
 return
 PropertyDictionary.Select(
 kvp => new {
 key = kvp.Key,
 value = (string) kvp.Value.GetValue(myClass, null)
 }
 )
 .Where(o => !string.IsNullOrEmpty(o.Value));
 // But now this isn't returning a Dictionary, but an anonymous object,
 // so this isn't going to work
}

and

private static IReadOnlyDictionary<string, string> GetPropertyValues(MyClass myClass) {
 return
 new ReadOnlyDictionary(
 PropertyDictionary.Select(
 kvp => new {
 key = kvp.Key,
 value = (string) kvp.Value.GetValue(myClass, null)
 }
 )
 .Where(o => !string.IsNullOrEmpty(o.value))
 .ToDictionary(o => o.key, o => o.value)
 );
}

Are either of these substantially superior?

Of course, all comments to improve quality are welcome.

A peripheral note on the Reflection shown here

Please stay focused on the requested aspects of the code review. For informational purposes: there are a number of properties in MyClass that have a special label accompanying them. This code is expected to be changed over time with new properties being added. Instead of the labels being associated with each property via a list or procedurally, and being far away in the code from the properties themselves, it seems best to me that each property simply be annotated via an attribute (right next to the property!) stating the property's key for serialization. Getting that attribute takes reflection. The PropertyInfo collection is materialized only once, and then used repeatedly at serialization time. If there is a performance problem, which I don't anticipate, I will use Reflection.Emit to solve it, because I think the value of the label-bearing attribute is high and leads to better and easier to maintain code.

• Dan Lyons: What is the result used for? Since you mention it is called by an instance method, is there a reason you are using reflection rather than pulling the values from fields/properties?
• ErikE: @DanLyons Please see my update in the question.

Answer 1

I think the cleanest way to optimize it and to use only one loop is to first get the values with a select, create key/value pairs there and then filter by value:

private static IEnumerable<KeyValuePair<string, string>> GetPropertyValues(MyClass myClass) {
 return
 PropertyDictionary
 .Select(kvp => 
 new KeyValuePair<string, string>
 (
 kvp.Key, 
 (string)kvp.Value.GetValue(myClass, null)
 )
 )
 .Where(kvp => !string.IsNullOrEmpty(kvp.Value));
}

Answer 2

Using an anonymous type is slower than using a concrete type. That being said, the third and fourth option shouldn't be used.

By not using one of the extension methods but doing this by a simple foreach loop over the KeyValuePair's of the IReadOnlyDictionary this will be more readable and faster, because you only need one loop like so

private static IEnumerable<KeyValuePair<string,string>> GetPropertyValues(MyClass myClass)
{
 foreach (KeyValuePair<string,PropertyInfo> kvp in PropertyDictionary)
 {
 string value = (string)kvp.Value.GetValue(myClass, null);
 if (String.IsNullOrEmpty(value)) continue;
 yield return new KeyValuePair<string, string>(kvp.Key, value);
 }
}

Answer 3

Since it's a private method it doesn't matter a whole lot what you do. You control all of the callers so as long as it works there's not a lot to criticize it for. Of course with an IEnumerable return value all you can do it iterate, but if that's all you were doing with the IReadOnlyDictionary then it's not a loss of functionality. There's also the argument for declaring a "minimal" return type to allow easier refactoring. So if you don't need the functionality of a dictionary, declare it to return the IEnumerable.

You're right that returning a Dictionary of an anonymous type in this instance will be a problem since the function must declare the return type. But you can just create KeyValuePairs instead, right?

Answer 4

I would prefer to use C# 7 Value Tuples:

private static IEnumerable<(string Key, string Value)> GetPropertyValues(MyClass myClass) {
 return
 PropertyDictionary
 .Select(kvp => 
 (
 kvp.Key, 
 Value:(string)kvp.Value.GetValue(myClass, null)
 )
 )
 .Where(kvp => !string.IsNullOrEmpty(kvp.Value));
}