current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 3 down vote favorite

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)) ? "" : " ";
share|improve this question
2  
Please change the title to what your code does. –  JaDogg 15 hours ago

4 Answers 4

up vote 8 down vote accepted

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);
share|improve this answer
    
There's also no need for the else's in the statements :) –  Simon 15 hours ago
1  
@Simon maybe not but since both options are equally important the else should stay –  ratchet freak 15 hours ago
    
@ratchetfreak Good answer, I only do not understand your reasoning in above comment. The else can be omitted as there are only two choices. –  Abbas 4 hours ago
    
@Abbas code-wise it doesn't matter, I know. But because space and empty are equally important, otherwise the if would be an exceptional value and the fall through path the default. –  ratchet freak 3 hours ago
up vote 4 down vote

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.

share|improve this answer
up vote 3 down vote

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".

share|improve this answer
up vote 1 down vote

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

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.