The first thing that pops out is the repetitive calls that look like this:
result = ((ExpandoObject)result).FirstOrDefault(x => x.Key == "node").Value;
Which are all similar. Half of your calls end in .Value and the other half don't. Which brings up a question on my part, since I don't understand var in C# well enough. So, you can put that in a separate function:
/**
* You'll have to fill in the return type and come up with a super
* cool name.
*/
var getObj(String key)
{
return ((ExpandoObject)result).FirstOrDefault(x => x.Key == key);
}
// Combine with an assignment in the condition statement, and
// you can do this:
if((result = getObj(key)) != null) { ... }
Upon further inspection, we can improve this slightly as well.
bool getObj(String key, out var result)
{
result = ((ExpandoObject)result).FirstOrDefault(x => x.Key == key);
return result != null;
}
// So you can do things like:
if(getObj(key, out result)) { ... }
The second thing that pops out is the quintuple nested if statement. The rule of thumb I've heard before is to have 2-3 nested loops/ifs at most. It looks like you're traversing an XML-like structure, in which case I would almost always go with a recursive method to find the child node you're looking for. In my experience, you do this by passing the object to search along with a queue of nodes. The only downside here is that you're looking for a specific piece of data in a specific place, so you have to know all of the steps to get there(which you do in this case).
// I'm making some assumptions here.
ExpandoObject find(Queue<String> queue, ExpandoObject tree)
{
if(getObj(queue.Dequeue, out tree))
{
return find(queue, tree);
}
else if (queue.Count > 0)
{
// We couldn't find an intermediate node, so we want
//to return null to show our failure
return null;
}
else
{
return tree; // May be null!
}
}
Finally, the IList seems to get in the way. It's unlike the other function calls you're making, because it's using the node's Value instead of its Key. However, if we revisit our getObj function, we might be able to resolve that. AS you can see below, if we pass it a key equal to "", we don't look up the key, but instead set the result to the Value instead.
bool getObj(String key, out var result)
{
if(key.Equals(""))// If there is no key, get the value
{
result = ((IList<object>)((KeyValuePair<string, object>)result).Value).FirstOrDefault();
}
else // If there is a key, use it!
{
result = ((ExpandoObject)result).FirstOrDefault(x => x.Key == key);
}
return result != null;
}
Then the question becomes, how do we call all of this to make it work? I'll start with the first line of your code:
dynamic model = GetExpandoObject(); //model type of ExpandoObject
Queue<String> queue;
queue.Enqueue("node");
queue.Enqueue("");
queue.Enqueue("children");
queue.Enqueue("");
queue.Enqueue("node");
queue.Enqueue("");
queue.Enqueue("stuff");
ExpandoObject result = find(queue, model);
if(result != null)
{
// Success! do what you will with it.
}
And the caveat, of course, is that I can't compile any of this, run it, or verify the sanity of any of it. At the very least, I hope it gives you some ideas or will work for you with a few tweaks.
It's my personal preference to always explicitly name the types of things in C#. dynamic and var are wonderful and have their place, but I only use them for one-liners. Carrying around a var for a long time through nested statements can be a pain to debug and maintain when you revisit this code.
Unfortunately, I don't know much about linq - well, I know it, but I haven't used it enough to have that intuition on when and how to use it really effectively - so I can't provide a solution that uses linq.
FirstOrDefaulton anIEnumerableof reference types can return null. Many of those.Valuecalls are vulnerable toNullReferenceExceptions. \$\endgroup\$ – Dan Lyons Mar 4 '14 at 18:32dynamic model = …; model.FirstOrDefault(…)How can this compile?FirstOrDefault()is an extension method and those don't work ondynamic. \$\endgroup\$ – svick Mar 4 '14 at 20:03