4
\$\begingroup\$

For my blazor library which is a modification of this awesome library I have to convert an ExpandoObject into a Dictionary<string, object> since ExpandoObjects aren't serialized properly in the newest preview versions of dotnet-core. See my question related to this for more details.

My current approach goes as follows:

Dictionary<string, object> ConvertDynamicToDictonary(IDictionary<string, object> value)
{
    return value.ToDictionary(
        p => p.Key,
        p =>
        {
            // if it's another IDict (might be a ExpandoObject or could also be an actual Dict containing ExpandoObjects) just go through it recursively
            if (p.Value is IDictionary<string, object> dict)
            {
                return ConvertDynamicToDictonary(dict);
            }

            // if it's an IEnumerable, it might have ExpandoObjects inside, so check for that
            if (p.Value is IEnumerable<object> list)
            {
                if (list.Any(o => o is ExpandoObject))
                {
                    // if it does contain ExpandoObjects, take all of those and also go through them recursively
                    return list
                        .Where(o => o is ExpandoObject)
                        .Select(o => ConvertDynamicToDictonary((ExpandoObject)o));
                }
            }

            // neither an IDict nor an IEnumerable -> it's probably fine to just return the value it has
            return p.Value;
        } 
    );
}

This works fine but I'm not sure about the IEnumerable part. If I omit the list.Any check it still works fine. New version (only write the IEnumerable part changed):

// if it's an IEnumerable, it might have ExpandoObjects inside, so check for that
if (p.Value is IEnumerable<object> list)
{
    // take all ExpandoObjects and go through them recursively
    return list
        .Where(o => o is ExpandoObject)
        .Select(o => ConvertDynamicToDictonary((ExpandoObject)o));
}

I have also tried using IEnumerable<ExpandoObject> instead of just IEnumerable<object> to see if that would work and maybe be cleaner and I can confirm that that does not work (throws error).

What I would like to know now is, if it's a good idea to omit the Any check and which option is more performant (and why) and/or cleaner.
Also please mention every small thing that bugs you, I always want to make it as perfect as possible :)

I first wanted to post this on StackOverflow but I think it fits better on here - correct me if I'm wrong.

EDIT 1:

Because I feel like I have not made clear how I will use this function, I have some demo-code for you. This will of course not compile, it's just to show how and why I need this method.
If this edit is problematic because I'm adding new code, I will post a new, more complete question later on (I sadly don't have anymore time today).

// this class (like all the others) are of course much more complicated than this. 
// There's also a lot of abstraction etc (you can see the actual code on the github I've linked at the start).
SomeConfig config = new SomeConfig
{
    Options = new SomeOptions
    {
        SomeInt = 2,
        SomeString = null,  // any trace of this property will not be present in the expando object
        Axes = new List<Axis>
        {
            new Axis()      // this axis will also be an expandoObject after being extracted from the json
        }
    },
    Data = new SomeData
    {
        Data = new List<int> { 1, 2, 3, 4, 5 },
        SomeString = "asdf"
    }
};

dynamic expando = /* ParseConfigToJsonWithoutNulls -> ParseJsonToExpandoObject */
IJSRuntime jsRT = GetTheRuntime();
jsRT.InvokeAsync("blabla", expando); // this would NOT work because ExpandoObject cannot be serialized to json correctly and throws a runtime error

// this method is what we need in the best way possible without missing something :)
Dictionary<string, object> dictFromExpando = ConvertExpandoToDictonary(expando);

// so the only purpose of the ConvertExpandoToDictonary is to recursively convert all the ExpandoObjects to Dictionaries as they are serializable
// I have to do this recursively since there are nested ExpandoObjects that were created when parsing the huge json to the ExpandoObject with Json.Net
// I have decided to go with Dictionaries because it works and ExpandoObject implements the interface IDictionary<string, object> 
jsRT.InvokeAsync("blabla", dictFromExpando); // this now works perfectly fine

EDIT 2:

I have asked a new, much more detailed question. Please have a look at it if you have the time :)

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Please see What to do when someone answers. I have rolled back Rev 3 → 2 \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jul 2 at 20:42
  • 1
    \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ Sorry I did not know. I feel like this was just a small mistake I made which may have caused some misunderstandings. Do I really have to ask a new question just for that (becaues I feel like my main concern didn't have to do with that at all) or how would you recommend me clearing that misunderstanding up to the concerned question? \$\endgroup\$ – Joelius Jul 2 at 20:45
  • 3
    \$\begingroup\$ I would await sufficient answers and comments. Then you could self-answer or accept if you please, and perhaps also ask a follow-up question. \$\endgroup\$ – dfhwze Jul 2 at 20:47
  • 2
    \$\begingroup\$ The rollback is being discussed on meta. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jul 2 at 21:10
  • 2
    \$\begingroup\$ Sure, it happens often enough you receive multiple very good answers making the decision which to pick a bit harder :-) \$\endgroup\$ – Mast Jul 3 at 12:02
7
\$\begingroup\$

Your method is meant to handle ExpandoObjects explicitly, so it would be better named ConvertExpandoObjectToDictonary, and would probably be less confusing if it actually took an ExpandoObject as the parameter:

Dictionary<string, object> ConvertExpandoObjectToDictonary(ExpandoObject expandoObject);

This is what the public API should look like; you can keep the current method as it is (since you need to call it recursively) but make it private and hide it behind the clearer public API.


This doesn't really make sense:

if (list.Any(o => o is ExpandoObject))
{
    // if it does contain ExpandoObjects, take all of those and also go through them recursively
    return list
        .Where(o => o is ExpandoObject)
        .Select(o => ConvertDynamicToDictonary((ExpandoObject)o));
}

You are looking for a single ExpandoObject, and then assuming everything you might need is an expando object and ignoring everything else. I think you want something like this instead, which keeps non-expando entries:

return list
    .Select(o => o is ExpandoObject
        ? ConvertDynamicToDictonary((ExpandoObject)o)
        : o
    );
\$\endgroup\$
  • \$\begingroup\$ You are enforcing a signature and flow based on a strict interpretation of the spec. Since the spec is lacking some context, I find this approach OK. \$\endgroup\$ – dfhwze Jul 2 at 20:37
  • 1
    \$\begingroup\$ @dfhwze sorry; I'm not sure what you mean? (I'm mostly going by the title, which suggests the purpose of this method is exactly to process ExpandoObjects) \$\endgroup\$ – VisualMelon Jul 2 at 20:40
  • \$\begingroup\$ The OP uses a dict both to process dictionaries and expando's. You took the ambiguity away. This has an impact on the algorithm, if there really are dictionaries to be processed that aren't expando's. The OP should perhaps make clear the usage of dict. \$\endgroup\$ – dfhwze Jul 2 at 20:44
  • 1
    \$\begingroup\$ @dfhwze I think I see what you mean. I'd maintain that if the intention is to convert ExpandoObjects (which is how I read the OP) then the API should reflect that; the work can be referred to a method that processes dictionaries (per the OP): it needn't have any influence on the algorithm. \$\endgroup\$ – VisualMelon Jul 2 at 20:50
  • 1
    \$\begingroup\$ yes :) that would be OK. Let's stop hijacking the comments of this answer now :p \$\endgroup\$ – dfhwze Jul 2 at 20:58
5
\$\begingroup\$

The root of the problem lies in this line:

dynamic expando = /* ParseConfigToJsonWithoutNulls -> ParseJsonToExpandoObject */

where you decided to parse *.json files into an ExpandoObject and not directly into a Dictionary or some other strong type.

I bet you are using Json.Net for the job and there are countless possibilities to deserialize JSON in such a way that ExpandoObject is not necessary.

This means that the conversion should take place during deserialization and not after that.


I suggest posting another question where you show us how you read your *.json. Maybe then we can help you to get rid of the ExpandoObject altogether.

\$\endgroup\$
  • 1
    \$\begingroup\$ I have asked a new question containing json-examples and more. Have a look at it :) \$\endgroup\$ – Joelius Jul 3 at 11:54
4
\$\begingroup\$

Inconsistent definition of ExpandoObject. It's IDictionary<string, object> here:

if (p.Value is IDictionary<string, object> dict)
{
    return ConvertDynamicToDictonary(dict); // <- possibly already visited?
}

and ExpandoObject here:

if (list.Any(o => o is ExpandoObject))
{
    // ..
}

I would opt to use IDictionary<string, object> both to improve consistency and flexibility of handling instances in the hierarchy.


It is not clear whether the structure of the hierarchical data is a tree or a cyclic graph. This has impact on a possible infinite recursion. In case of a cyclic graph, I would keep track of visited references.


Not all sequences are generic classes.

This condition..

if (p.Value is IEnumerable<object> list)

..does not cover when p.Value is IEnumerable but not IEnumerable<object>. As note in another answer, be careful with false positives (like string).


What if value is null? Try avoiding null reference exceptions in your code.


\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the ideas. The ExpandoObject should always be a tree since it is just an instance of a class with all traces of null properties removed. This is necessary for correctly invoking some Javascript. \$\endgroup\$ – Joelius Jul 2 at 20:55
2
\$\begingroup\$

A few minor stylistic bits that may increase its utility:

  • since you take in an IDictionary, return an IDictionary.
  • make it static as it accesses no instance data or methods.
  • since it's now static, make it an extension method.
  • validate the parameter passed in.

Also:

  • since it's a single return statement, make it an expression body.

Further:

  • yeah, the .Any() is redundant (and less performant if there is an ExpandoObject in the collection) with the .Where() criteria, so no need for it.
  • allowed for non-generic IEnumerable.
  • renamed some of the locals to be clearer as to their meaning.

Giving:

static IDictionary<string, object> ConvertDynamicToDictionary(this IDictionary<string, object> source) => source?.ToDictionary(
    keySelector => keySelector.Key,
    elementSelector =>
    {
        object value = elementSelector.Value;

        // if it's another IDict (might be a ExpandoObject or could also be an actual Dict containing ExpandoObjects) just go through it recursively
        if (value is IDictionary<string, object> dictionary)
        {
            return dictionary.ConvertDynamicToDictionary();
        }

        // A special case since string implements IEnumerable.
        if (value is string stringValue)
        {
            return stringValue;
        }

        // if it's an IEnumerable, it might have ExpandoObjects inside, so check for that
        if (value is IEnumerable enumerable)
        {
            // if it does contain ExpandoObjects, take all of those and also go through them recursively
            return enumerable
                .Cast<object>()
                .Select(element => element is IDictionary<string, object> expando
                    ? expando.ConvertDynamicToDictionary()
                    : element);
        }

        // neither an IDict nor an IEnumerable -> it's probably fine to just return the value it has
        return value;
    });
\$\endgroup\$
  • \$\begingroup\$ Ugh, string implements IEnumerable. \$\endgroup\$ – Jesse C. Slicer Jul 2 at 20:36
  • \$\begingroup\$ Clear points, thank you. Yes I already wanted to point out that IEnumerable is used seemingly everywhere and ask if that would cause issues (it would just decrease the performance right?). Also I have edited my question regarding the signature because the method is already private and static I just forgot to write that. \$\endgroup\$ – Joelius Jul 2 at 20:39
  • \$\begingroup\$ It's not that IEnumerable decreases performance, it's that the .Any() followed by .Where() with the same lambda does the same checking, and therefore .Any() is redundant. \$\endgroup\$ – Jesse C. Slicer Jul 2 at 20:44
  • 1
    \$\begingroup\$ @JesseC.Slicer are you okay with this revision to the question which would affect your answer? If so, we could re-apply it... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jul 2 at 20:47
  • 1
    \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ I think the revision is minor enough, I could remove a single line from my answer to keep it salient. \$\endgroup\$ – Jesse C. Slicer Jul 2 at 20:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.