views:

254

answers:

6

Because this is my first attempt at an extension method that seems quite useful to me, I just want to make sure I'm going down the right route

 public static bool EqualsAny(this string s, string[] tokens, StringComparison comparisonType)
    {
        foreach (string token in tokens)
        {
            if (s.Equals(token, comparisonType))
            {
                return true;
            }
        }

        return false;
    }

Called by

if (queryString["secure"].EqualsAny(new string[] {"true","1"}, StringComparison.InvariantCultureIgnoreCase))
{
    parameters.Protocol = Protocol.https;
}

EDIT: Some excellent suggestions coming through, exactly the sort of thing I was looking for. Thanks

EDIT:

I have decided on the following implementation

public static bool EqualsAny(this string s, StringComparison comparisonType, params string[] tokens)
{
    // for the scenario it is more suitable for the code to continue
    if (s == null) return false;

    return tokens.Any(x => s.Equals(x, comparisonType));
}

public static bool EqualsAny(this string s, params string[] tokens)
{
    return EqualsAny(s, StringComparison.OrdinalIgnoreCase, tokens);
}

I preferred using params over IEnumerable because it simplified the calling code

if (queryString["secure"].EqualsAny("true","1"))
{
    parameters.Protocol = Protocol.https;
}

A far cry on the previous

if (queryString["secure"] != null)
{
    if (queryString["secure"] == "true" || queryString["secure"] == "1")
    {
        parameters.Protocal = Protocal.https;
    }
}

Thank you again!

+2  A: 

Make your tokens parameter more general – i.e. make it an IEnumerable<string>.

Also, an equivalent method already exists that extends IEnumerable<>, e.g. Any:

 public static bool EqualsAny(this string s, IEnumerable<string> tokens, StringComparison comparisonType)
 {
     return tokens.Any(t => s.Equals(t, comparisonType));
 }

Also, Joel is of course right: you might want to check for null values before performing the actions (defensive coding). This isn't more secure but it makes the error easier to localize.

Konrad Rudolph
@Joel: you're right. I was talking more generally about *any* kind of error handling. Throwing `NullReferenceException` might be more appropriate. As you said: we don't know which behaviour is preferred here.
Konrad Rudolph
Eh, I meant `ArgumentNullException`.
Konrad Rudolph
I'd say that yes, it's definitely less secure, and possibly also still harder to localize, because the code could run with an invalid false assumption for a long time before the error surfaces in a visible way. Throwing an exception may be more appropriate, but we can't know which behavior the OP needs when writing the sample. Thus for better or worse the bias is to provide code that just runs.
Joel Coehoorn
Oops: 'edited' my comment (delete/replace cycle) while you were adding yours :(
Joel Coehoorn
+1  A: 

in order to simplify usage of EqualsAny you could use varargs and default strategy for StringComparison:

public static bool EqualsAny(this string s, params string[] tokens) {
    return EqualsAny(s, StringComparison.InvariantCultureIgnoreCase, tokens);
}

public static bool EqualsAny(this string s, 
                             StringComparison stringComparison, 
                             params string[] tokens) {
    // your method
}

Called by

if (queryString["secure"].EqualsAny("true", "1")) {
    parameters.Protocol = Protocol.https;
}
dfa
params has to be the last argument.
Will
+7  A: 

Yes! First, you need to check s for null. Also, let it accept any IEnumerable<string> for tokens rather than just an array, and then use other linq operators to do the check:

public static bool EqualsAny(this string s, IEnumerable<string> tokens, StringComparison comparisonType)
{
    if (s== null) return false;
    return tokens.Any(t => s.Equals(t, comparisonType));
}


Thinking about how to handle a null value for s, there's a third option no one's used yet:

 public static bool EqualsAny(this string s, IEnumerable<string> tokens, StringComparison comparisonType)
{
    if (s== null) return tokens.Any(t => t == null);
    return tokens.Any(t => s.Equals(t, comparisonType));
}


Finally, regarding your chosen implementation: if you're going to have overloads, you might as well have IEnumerable overloads as well, and have your params code call those.

Joel Coehoorn
I agree that it should be checked for null, but I'm not sure that it should always return false on an empty string.
Greg D
Yeah, I thought of that after I saw the post in the live view rather then a preview window- updated
Joel Coehoorn
if(string == null)?
Will
:( Fixed, thanks.
Joel Coehoorn
+5  A: 
public static bool EqualsAny(
    this string s, 
    StringComparison comparisonType, 
    params string[] tokens)
{
    foreach (string token in tokens)
    {
        if (s.Equals(token, comparisonType))
        {
            return true;
        }
    }
    return false;
}

With params, you don't have to force your strings into an array first.

var match = "loool".EqualsAny(StringComparison.Ordinal, "hurf", "Durf");

Linq-ified (JC + me) with a NRE (framework standard):

public static bool EqualsAny(
    this string s, 
    StringComparison comparisonType, 
    params string[] tokens)
{
   if(s == null) throw new NullReferenceException("s");
   return tokens.Any(x=> s.Equals(x, comparisonType));
}
Will
+3  A: 

Another option would be. This will simplify your call site since if you have a couple of strings your matching against you won't have to create the array or list in code.

 public static bool EqualsAny(this string s,StringComparison comparisonType, param string[] tokens )
{
   return EqualsAny(s,comparisonType,tokens);
}    

 public static bool EqualsAny(this string s,StringComparison comparisonType, IEnumerable<string>tokens )    
{ 
    //Throw nullReference to keep the semantics aligned with calling an instance member
    if (s==null) throw new NullReferenceException();       
    foreach (string token in tokens)        
    {            
         if (s.Equals(token, comparisonType))            
         {                
             return true;            
         }        
   }        
   return false;    

}
JoshBerke
A: 

There's nothing wrong per-se with what you're doing. However this type of functionality already exists in several different fashions.

Example:

var candidates = List<SomeObject>(); 
if (candidates.Count(c=> string.Compare(c.PropertyValue, queryString["secure"], StringComparison.InvariantCultureIgnoreCase) == 0) > 0)
{
 parameters.Protocol = Protocol.https;
}
Will Hughes
Yeh, but if you're doing it a lot then the helper extension method provides much more readable code - and that in itself has a lot of value.
joshcomley