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.

I have to loop through a list of strings and validate that all are parsable to int and not less than 1. Also, if a string is not parsable but is null or whitespace, then it is OK and my validator should not fail.

Here's the code I have come up with which works fine but leads to method complexity:

public static bool ArePerPersonBILimitsInvalid(IEnumerable<string> bilimits)
        {
            return bilimits.Any(c =>
            {
                int d;
                if (!int.TryParse(c, out d))
                {
                    if (string.IsNullOrWhiteSpace(c))
                    {
                        return false;
                    }

                       return true;
                }

                if (d < 1)
                {
                    return true;
                }

                return false;
            });
        }
share|improve this question

4 Answers 4

up vote 19 down vote accepted

I find that negatives in method names lead to confusing code. For instance,

if (!ArePerPersonBILimitsInvalid(...))

"Are there no BI limits that are invalid?"

becomes much more readable when we write it this way:

if (ArePerPersonBILimitsValid(...))

"Are all BI limits valid?"

So let's fill in the blanks.

public static bool ArePerPersonBILimitsValid(IEnumerable<string> biLimits)
{
    return biLimits.All(IsBILimitValid);
}

Great, so now we need to write IsBILimitValid.

public static bool IsBILimitValid(string biLimit)
{
    if (string.IsNullOrWhiteSpace(biLimit))
    {
        return true;
    }

    int value;
    return int.TryParse(biLimit, out value) && value >= 1;
}

This way the code much more closely reflects your problem statement:

loop through a list of strings and validate that all are parsable to int and not less than 1

share|improve this answer
    
Did you mean is less readable as instead of is more readable as ? –  Heslacher Aug 8 at 5:25
    
@Heslacher, no I meant "more readable (when written) as"... perhaps I should have said, "is less readable than". –  mjolka Aug 8 at 5:27
    
Still a "negative" with !int.TryParse. I'd go with return int.TryParse(biLimit, out value) && value >= 1; but it's debatable, if that's more readable. –  Corak Aug 8 at 5:53
    
@Corak that's a nice version, mind if I use that in my answer? My note about negatives was just in relation to method names, e.g. IsValid vs IsInvalid, HasErrors vs HasNoErrors. –  mjolka Aug 8 at 5:55
1  
@dreza thanks, I thought it was clear but getting two questions on that point meant it wasn't! I've tried to improve the wording. –  mjolka Aug 8 at 23:01

As per my interpretation, you don't need empty string check inside the if clause you can put it out side, and last condition could be expressed in one line.

return bilimits.Any(inputstring =>
{
    if (string.IsNullOrWhiteSpace(inputstring))
        return false;

    int parsedNumber;
    if (!int.TryParse(inputstring, out parsedNumber))
        return true;

    return parsedNumber < 1;
});
share|improve this answer

I don't think it's that really bad, but you could simplify it by doing it all in one statement.

public static bool ArePerPersonBILimitsInvalid(IEnumerable<string> bilimits)
{
    return bilimits.Any(c =>
    {
        int asInt;
        return (int.TryParse(c, out asInt) && asInt < 1) ||
                !string.IsNullOrWhiteSpace(c);
    });
}

Some of the other answers I actually like. I'll just provide this as an alternative.

share|improve this answer
1  
You don't need to initialize asInt, it's definitely assigned in all code after TryParse(). –  svick Aug 8 at 13:45
    
@svick true, just habit I think. –  dreza Aug 8 at 19:22
public static bool ArePerPersonBILimitsValid(this IEnumerable<string> biLimits)
{
    int intResult;
    var invalidResults = biLimits.Where(s => {
            if (int.TryParse(s, out intResult))
                return false;
            else
                return !string.IsNullOrWhiteSpace(s);
        });
    return invalidResults.Count() == 0;
}

string[] inValidTestIntegers = new string[] { "10", "12", "str" });
string[] balidTestIntegers = new string[] { "10", "12" });
bool isValid1 = inValidTestIntegers.ArePerPersonBILimitsValid();
bool isValid2 = balidTestIntegers.ArePerPersonBILimitsValid();

//output: false true

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.