Checking dictionary for specified key

Question

The first part of code is for me very unreadable. I tried to write in another way but I'm open to your suggestion!

return ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName)).Value != null && ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName)).Value.Errors.Any(x => !string.IsNullOrEmpty(x.ErrorMessage)) ? "" : " ";

//Looks in a dictionary for a key called parameterName
    ModelState modelState = ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName)).Value;
    //If I could find it I look for any error associatd to that and I return it as a single string
    return modelState != null && modelState.Errors.Any(x => !string.IsNullOrEmpty(x.ErrorMessage)) ? "" : " ";

Answer 1

Not only is this difficult to read, it's also inefficient.

Cache your duplicate LINQ lookup to save time and make it more readable.

As for using the ternary operator, that's up to you, but I prefer to use a full if statement when the line gets too long.

Additionally swap out "" for String.Empty, it shows your intent better, and also makes it more strongly contrast against " "

var state =ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName));

if(state.Value != null && 
    state.Value.Errors.Any(x => !string.IsNullOrEmpty(x.ErrorMessage)))
{
    return string.Empty;
}
else
{
    return " ";
}

I'm assuming this next bit executes elsewhere, because the two return statements above would otherwise make this unreachable code.

Use var when the type is obvious from the right hand side of the assignment (although in this case it's a little debatable, I'd personally use var here because the variable name mimics the type).

You have a typo in your second comment.

//Looks in a dictionary for a key called parameterName
var modelState = ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName)).Value;
//If I could find it I look for any error associated to that and I return it as a single string
if(modelState.Value != null && 
    modelState.Value.Errors.Any(x => !string.IsNullOrEmpty(x.ErrorMessage)))
{
    return string.Empty;
}
    else
{
    return " ";
}

Finally, structure-wise I'd recommend converting the if-statement above into a separate method, since you call it in multiple places. This will leave a single point to modify during refactoring.

private string ConvertModelStateErrorsToString(ModelState modelState)
{
    if(modelState.Value != null && 
        modelState.Value.Errors.Any(x => !string.IsNullOrEmpty(x.ErrorMessage)))
    {
        return string.Empty;
    }
        else
    {
        return " ";
    }
}

Leaving your other code samples as:

var state =ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName));

return ConvertModelStateErrorsToString(state);

And the other sample:

//Looks in a dictionary for a key called parameterName
var modelState = ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName)).Value;
//If I could find it I look for any error associated to that and I return it as a single string
return ConvertModelStateErrorsToString(modelState);

Answer 2

In addition to the answer of André.

Try making the return-statement as clear as possible, always! It's not easy to read such a line where you will return something after a long line of evaluations, certainly when using the ternary conditional expression (x?a:b).

Place the results of the evaluation in meaningful variables so when you read your code, you know what it's supposed to do. Your code might look something like this:

var modelState = ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName)).Value;
var isNotNull = modelState != null;
var hasErrors = modelState.Errors.Any(x => !String.IsNullOrEmpty(x.ErrorMessage));
return (isNotNull && hasErrors) ? "" : " ";

Now you can easily read the return-statement as:

Return an empty string when modelState is not null and has errors, otherwise return a space.

Note: I use the var keyword, but you may choose to use bool instead. Due to the fact that I let my bool-variables begin with is or has, I always know it's a boolean.

Answer 3

you should probably move it into a if-else statement:

if(modelState != null && modelState.Errors.Any(x => !string.IsNullOrEmpty(x.ErrorMessage)))
    return "";
else
    return " ";

This immediately say "this function returns a empty string or a string with a space depending on this condition".

Answer 4

In my opinion, it can be more readable if you separate the comparisons into boolean variables and document them.

Answer 5

return ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName)).Value != null && ViewData.ModelState.FirstOrDefault(x => x.Key.Equals(parameterName)).Value.Errors.Any(x => !string.IsNullOrEmpty(x.ErrorMessage)) ? "" : " ";

There is a reason that ModelStateDictionary implements IDictionary<string, ModelState> and not just IEnumerable<KeyValuePair<string, ModelState>>, and it's that by using it as a dictionary you get an efficient lookup. FirstOrDefault is a linear scan; a lookup in a dictionary should be near-constant (with a good hash) or logarithmic (if backed by a tree structure) in most cases.

const string okReturnValue = " ";
const string errorReturnValue = string.Empty;
ModelState parameterState;
return ViewState.ModelState.TryGetValue(parameterName, out parameterState) && parameterState.Errors.Any(x => !string.IsNullOrEmpty(x.ErrorMessage)) ? errorReturnValue : okReturnValue;

will perform consistently regardless of the number of other key-value pairs in ViewData.ModelState.