Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is it possible to simplify the code below?

dynamic model = GetExpandoObject(); //model type of ExpandoObject

var result = model.FirstOrDefault(x => x.Key == "node").Value;

if (result != null)
{
    result = ((ExpandoObject)result).FirstOrDefault(x => x.Key == "children");

    if (result != null)
    {
        result = ((IList<object>)((KeyValuePair<string, object>)result).Value).FirstOrDefault();

        if (result != null)
        {
            if (result != null)
            {
                result = ((ExpandoObject)result).FirstOrDefault(x => x.Key == "node").Value;

                if (result != null)
                {
                    result = ((ExpandoObject)result).FirstOrDefault(x => x.Key == "stuff");
                }
            }
        }
    }
}

Maybe it is possible to implement similar logic with Linq? Any thoughts?

share|improve this question
1  
please let us know if we are on the right track here @sreginogemoh –  Malachi Mar 4 at 16:22
1  
Keep in mind that FirstOrDefault on an IEnumerable of reference types can return null. Many of those .Value calls are vulnerable to NullReferenceExceptions. –  Dan Lyons Mar 4 at 18:32
    
dynamic model = …; model.FirstOrDefault(…) How can this compile? FirstOrDefault() is an extension method and those don't work on dynamic. –  svick Mar 4 at 20:03

5 Answers 5

FirstOrDefault is a LINQ extension method (see ExpandoObject Extension Methods), so you are using LINQ!

@Ryan's answer looks like it's addressing the structural issue with your code, got my +1.

I'd like to add that you should avoid reusing the same variable for a different meaning.

It's very hard for me to tell, but it looks like the code is abusing dynamic - the code compiles and runs because the type of result is dynamic and thus resolved at run-time, and yet you're taking every opportunity at casting it back into an ExpandoObject, so that you get compile-time knowledge of the type you're working with.

Doing this would "cut" the dynamic and have var evaluate to ExpandoObject straight from the start:

var result = model.FirstOrDefault(x => x.Key == "node").Value as ExpandoObject;

and then you could simply do:

if (result != null)
{
    result = result.FirstOrDefault(x => x.Key == "children");

However the constant re-assignation of result to a different value is not helping readability, I suggest you put a meaningful name to each step.

share|improve this answer

In your code, the variable result is just of type object because the compiler doesn't know what the result of FirstOfDefault is going to be.

Below I've used explicit typing using T for clarity and so that var doesn't evaluate to object

I created this extension method so that you wouldn't have to cast the object everytime you wanted to use it... this takes care of that before it returns

public static class ExtensionMethods
{
    public static T FirstOrDefault<T>(this ExpandoObject eo, string key)
    {
        object r = eo.FirstOrDefault(x => x.Key == key).Value;
        return (r is T) ? (T)r : default(T);
    }
}

What Follows is a re-implementation of the original post's code using the above method

ExpandoObject model = GetExpandoObject();

var someExpandoObject = model.FirstOrDefault<ExpandoObject>("node");
if (someExpandoObject == null) return;

var kvp = someExpandoObject.FirstOrDefault<KeyValuePair<string, object>>("children");
if (kvp.Equals(default(KeyValuePair<string, object>))) return;

var ilo = kvp.Value as IList<object>;
if (ilo == null) return;

someExpandoObject = ilo.FirstOrDefault() as ExpandoObject;
if (someExpandoObject == null) return;

someExpandoObject = someExpandoObject.FirstOrDefault<ExpandoObject>("node");
if (someExpandoObject == null) return;

object finalResult = someExpandoObject.FirstOrDefault<object>("stuff");

What follows is another possible implementation using ifs instead of returns.

ExpandoObject model = GetExpandoObject();

ExpandoObject someExpandoObject;
KeyValuePair<string, object> kvp;
IList<object> ilo;
object finalResult;

if ((someExpandoObject = model.FirstOrDefault<ExpandoObject>("node")) != null)
{
    if (!(kvp = someExpandoObject.FirstOrDefault<KeyValuePair<string, object>>("children")).Equals(default(KeyValuePair<string, object>)))
    {
        if ((ilo = kvp.Value as IList<object>) != null)
        {
            if ((someExpandoObject = ilo.FirstOrDefault() as ExpandoObject) != null)
            {
                if ((someExpandoObject = someExpandoObject.FirstOrDefault<ExpandoObject>("node")) != null)
                    finalResult = someExpandoObject.FirstOrDefault<object>("stuff");
            }
        }
    }
}
share|improve this answer
    
the way I understand the OP is that they want the deepest node.value that exists. your code doesn't look like it will accomplish this. but is good sounds code. –  Malachi Mar 4 at 16:18
2  
Congratulations, you've earned my last vote for the day! Good job at removing all the if blocks :) –  Mat's Mug Mar 4 at 16:21
    
@Malachi I'm not sure what the OP is trying to do exactly... a diagram of this object structure would have been helpful. I guess if he needs to go deeper he can loop :D. –  BenVlodgi Mar 4 at 16:33
    
@Mat'sMug Thanks! It is shorter code, and less messy that way... as-long as he doesn't want early returns, it is the way to go. –  BenVlodgi Mar 4 at 16:33

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.

share|improve this answer
2  
var has nothing to do with dynamic. var is simply implicit typing - the actual type is inferred from the type of the assignment, and that's resolved at compile-time (you get IntelliSense and can even get the IDE to tell you what var stands for by hovering the variable with your mouse cursor). dynamic, however, is resolved at run-time. That's something you don't want to carry around for a long time through nested statements. Using var has zero caveats and reduces redundant code like MyObject foo = new MyObject(). –  Mat's Mug Mar 4 at 14:37
1  
@Mat'sMug Thanks for the clarification! I'll admit, C# is a language I only use in my spare time(and wish I could use it on the job). –  Ryan Mar 4 at 16:14
    
Combining bool and out makes sense when the default value is also valid. But here, null indicates failure, so there is no reason for the separate bool result. –  svick Mar 4 at 20:12

you could make it a little less cluttered by putting the result into the if statement like this

dynamic model = GetExpandoObject(); //model type of ExpandoObject

var result = model.FirstOrDefault(x => x.Key == "node").Value;

if (result = null)
{
}else{
    if(((ExpandoObject)result).FirstOrDefault(x => x.Key == "children") = null)
    {
        //result is already what it should be.
    } else {
        if (((IList<object>)((KeyValuePair<string, object>)result).Value).FirstOrDefault() = null)
        {
            result = ((ExpandoObject)result).FirstOrDefault(x => x.Key == "children");
        } else {
            if (((ExpandoObject)result).FirstOrDefault(x => x.Key == "node").Value = null)
            {
                result = ((IList<object>)((KeyValuePair<string, object>)result).Value).FirstOrDefault();
            } else {
                result = ((ExpandoObject)result).FirstOrDefault(x => x.Key == "stuff");
            }
        }
    }
}

the innermost else statement should still check ((ExpandoObject)result).FirstOrDefault(x => x.Key == "stuff") for nulls depending on what you are doing with the result. if something else checks for nulls later than I guess you shouldn't worry about it here.

the way I did it here makes sure that you are really only assigning to the variable twice at most, the first initialization of the variable and then again if there is a deeper node that needs to be assigned to it.

share|improve this answer
2  
C# won't cast references to bool as a test-for-null: it needs explicit comparison i.e. != null –  ChrisW Mar 4 at 15:54
    
@ChrisW, that should work then, you could get rid of the variable all together –  Malachi Mar 4 at 15:58
1  
And now you got caught by the reuse of result - it needs to be reassigned for it to work. This code isn't equivalent to the OP's. –  Mat's Mug Mar 4 at 16:01
1  
I think your newer code doesn't work because, unlike the OP, each statement isn't reassigning to / reusing the result variable. In fact you're using result in your expressions but you deleted its first definition/assignment. –  ChrisW Mar 4 at 16:02
    
@Mat'sMug I was just looking at that. give me a second guys... –  Malachi Mar 4 at 16:02

I would take advantage of using dynamic. It will throw RuntimeBinderException when a property doesn't exist, so you can just catch that:

dynamic model = GetExpandoObject();

dynamic result = null;

try
{
    result = model.node.children.node.stuff;
}
catch (RuntimeBinderException)
{
}

Using exceptions unnecessarily is not great, but I think it's better than your overly verbose code.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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