Condition always evaluating to true

[Solarion]

Is this what you call a false positive?

public static bool ValidatePhoneNumber(this string phone, bool IsRequired)
{
    if (string.IsNullOrEmpty(phone) && !IsRequired)
        return true;

    if (string.IsNullOrEmpty(phone) && IsRequired)
        return false;

    var cleaned = phone.RemoveNonNumeric();
    if (IsRequired)
    {
        if (cleaned.Length == 10)
            return true;
        else
            return false;
    }
    else
    {
        if (cleaned.Length == 0)
            return true;
        else if (cleaned.Length > 0 && cleaned.Length < 10)
            return false;
        else if (cleaned.Length == 10)
            return true;
        else
            return false; // should never get here
    }
}

 

[randomcat]

I dont think the second if statement is needed

 

[swift412]

It must be the rare and elusive true-able bool, distance relative of the nullable bool, implemented as:

public bool IsRequired { get => true; set {}}

On a more serious note, I don't get a similar warning in VS Code. Also, if you're on C#8.0, tuples and switch expressions are pretty neat:

public static bool ValidatePhoneNumber (this string phone, bool IsRequired) {
    if (string.IsNullOrWhitespace(phone)) return !IsRequired;
    var cleaned = phone.RemoveNonNumeric ();
    return (IsRequired, cleaned.Length == 0, cleaned.Length == 10) switch {
        (true, _, true) => true,
        (false, true, _) => true,
        (false, _, true) => true,
        _ => false
    };
}

 

[scudsucker]

Also, if you're on C#8.0, tuples and switch expressions are pretty neat:

Wow, yes, that is cool. I've been away from .NET for a few years; that switch is lovely.

 

[Solarion]

It must be the rare and elusive true-able bool, distance relative of the nullable bool, implemented as:

public bool IsRequired { get => true; set {}}

On a more serious note, I don't get a similar warning in VS Code. Also, if you're on C#8.0, tuples and switch expressions are pretty neat:

public static bool ValidatePhoneNumber (this string phone, bool IsRequired) {
    if (string.IsNullOrWhitespace(phone)) return !IsRequired;
    var cleaned = phone.RemoveNonNumeric ();
    return (IsRequired, cleaned.Length == 0, cleaned.Length == 10) switch {
        (true, _, true) => true,
        (false, true, _) => true,
        (false, _, true) => true,
        _ => false
    };
}

That is really nice! I can't use it yet until I install C#8.0 but it looks great. Also my apologies I forgot to add one of the functions in my OP. This is mostly for if you have a mask on the input eg. 031-9061254

public static string RemoveNonNumeric(this string phone)
{
    return Regex.Replace(phone, @"[^0-9]+", "");
}

 

[_kabal_]

Is this what you call a false positive?

public static bool ValidatePhoneNumber(this string phone, bool IsRequired)
{
    if (string.IsNullOrEmpty(phone) && !IsRequired)
        return true;

    if (string.IsNullOrEmpty(phone) && IsRequired)
        return false;

    var cleaned = phone.RemoveNonNumeric();
    if (IsRequired)
    {
        if (cleaned.Length == 10)
            return true;
        else
            return false;
    }
    else
    {
        if (cleaned.Length == 0)
            return true;
        else if (cleaned.Length > 0 && cleaned.Length < 10)
            return false;
        else if (cleaned.Length == 10)
            return true;
        else
            return false; // should never get here
    }
}

seems legit to me

it cannot be anything other than true because you already tested if it is false with the same precondition before (is the string null or empty)

Read it like this and it will be obvious

if (true && false) { 
   return true;
}
if (true && true) {
      return false;
}
//So now, kind of like “solve for x”, remove the duplicates, and you get
if (false) { 
   return true;
}
if (true) {
      return false;
}
//which is basically 
if (false) { 
   return true;
} else {
    return false;
}
// OR
if (false) { 
   return true;
}
return false;

 

[sn3rd]

seems legit to me

it cannot be anything other than true because you already tested if it is false with the same precondition before (is the string null or empty)

Read it like this and it will be obvious

if (true && false) {
   return true;
}
if (true && true) {
      return false;
}
//So now, kind of like “solve for x”, remove the duplicates, and you get
if (false) {
   return true;
}
if (true) {
      return false;
}
//which is basically
if (false) {
   return true;
} else {
    return false;
}
// OR
if (false) {
   return true;
}
return false;

Agree with this. If you nested, it would be even clearer:

public static bool ValidatePhoneNumber(this string phone, bool IsRequired)
{
    if (string.IsNullOrEmpty(phone) {
        if (!isRequired) {
            return true;
        }
        if (isRequired) { //shouldn't ever be anything else; so why bother checking?
            return false;
        }
    }
    ...
    ...
}

Perhaps not my place, but style-wise, something like the following "feels" better to me:

public static bool ValidatePhoneNumber(this string phone, bool isRequired)
{
    if (string.IsNullOrEmpty(phone) {
        return !isRequired;
    }
    ...
    ...
}

 

[[)roi(]]

Perfect case where to use a nullable type.
If you're not using C# 8.0; then create you own Maybe type and use that.

For an example; here is my implementation of Maybe type in C#; however you can massively simplify that if you don't need all the function programming algebras. However using the algebras can substantially shorten and simply the validation code.