Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String.Equals isn't intuitive and forces us to do the OrdinalIgnoreCase thing #14065

Open
shanselman opened this issue Feb 5, 2015 · 46 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Milestone

Comments

@shanselman
Copy link

We're doing a sample and we are FOREVER doing this

if (sort.Equals("Title", StringComparison.OrdinalIgnoreCase))

I think it's time for an overload or an extension that "does the right thing." This is a 10 year old pattern and if the default is wrong (as we tell people to do this) then let's make a better method. Everyone has a version and extension that does this.

@jongalloway
Copy link
Contributor

I agree +10e8

@madskristensen
Copy link

+1

This drives me nuts and forces me to do multi-line if statements due to very long lines this produces. I hate that. A better and much shorter syntax would be super awesome!

@rustd
Copy link

rustd commented Feb 5, 2015

the default should really be changed as most apps have to have this check

@Bhaal22
Copy link

Bhaal22 commented Feb 5, 2015

+1
Makes me crazy.

@csharpfritz
Copy link

Agreed - lets help our community by making the framework use the correct pattern by default

@joshfree
Copy link
Member

joshfree commented Feb 5, 2015

var oic = StringComparer.OrdinalIgnoreCase;

if (oic.Equals(sort,"Title"))

@ellismg
Copy link
Contributor

ellismg commented Feb 5, 2015

I'm not pushing back against this request, but does someone have a strawman to kick off the design? The best thing I can think of is: EqualsOrdinalIgnoreCase as a method on String. Is this still too long?

@shanselman
Copy link
Author

Why not EqualsIgnoreCase. Is Ordinal that obvious, or needed? (although semantically it would be implied.)

@ellismg
Copy link
Contributor

ellismg commented Feb 5, 2015

@shanselman I guess that's reasonable. Personally I dislike how some operations on the string class do linguistic comparisons where as others do ordinal and it's not clear from the method name what's going on. This proposal would be making the problem worse, but perhaps we should optimize for folks that have already learned for each method if it's doing ordinal or linguistic conversions.

@shanselman
Copy link
Author

@ellismg I hear you. I'm trying to optimize for the 80% or even the 90% case. What is the "pit of success" option? What's the one that if folks who don't know the difference pick is more likely to be correct?

@plemanach
Copy link

+1 EqualsIgnoreCase

@davkean
Copy link
Member

davkean commented Feb 6, 2015

@rustd Are you suggesting that ignore case should be the default? Changing defaults around this is pretty much a non-starter. We've tried similar things in the past (such as switching all operations to ordinal and invariant by default in Silverlight) but ended up backing it out for a variety of reasons. There's just too much legacy code being copied and pasted around.

I like static bool EqualsIgnoreCase(string, string) and bool EqualsIgnoreCase(string), it fits in with existing overloads static bool Equals(string, string) and bool Equals(string) which already do a ordinal comparison.

@ebickle
Copy link

ebickle commented Feb 6, 2015

@davkean @shanselman Agree on EqualsIgnoreCase.

I did a quick scan through the String class, other methods that take a StringComparison overload include Compare, CompareOrdinal, CompareTo, StartsWith, EndsWith, IndexOf, LastIndexOf, Replace. None of them seem like a high priority for an IgnoreCase overload.

What's painfully missing is a Contains overload that accepts a comparison type. It's a bigger functionality gap than EqualsIngoreCase.

public bool EqualsIgnoreCase(string value);
public static bool EqualsIgnoreCase(string a, string b);
public bool Contains(string value, StringComparison comparisonType);

This may be a bit more of a stretch, a new IStringEquatable interface could be introduced that inherits from IEquatable. IStringEquatable would add the necessary overloads for the comparison type.

public IStringEquatable<T> : IEquatable<T>
{
    bool Equals(T other, StringComparison comparisonType);
}

The C# team is currently looking at potentially implementing pattern matching and improvements to the switch statement for C# 7.0. One source of pain has been the inability to do case insensitive comparisons in a switch statement - introducing the interface would provide a hook for case insensitive comparisons as 'syntactic sugar'. The interface could be added to the other string-like types, such as HtmlString.

I never quite understood why languages don't introduce a case-insensitive string comparison operator. It's likely that because it only applies to a string, it doesn't meet the bar required for a broadly used language feature. That said, the same was once true for String types themselves (just use an array!). Case-insensitive string comparisons are a very bread & butter development operation, and as noted above can apply to types other than System.String.

Imagine the theoretical ==~ and !=~ operators (the tilde being the approximation symbol, not bitwise complement):

public static String overload ==~(String a, String b)
public static String overload !=~(String a, String b)

if ("CoreFx" ==~ "corefx") // true

Even aside from theoretical language changes (very unlikely), the interface would allow libraries such as LINQ to leverage case-insensitive string equality natively without resorting to reflection. The same holds true for String.Compare, which strangely is missing a non-static String.Compare(string, StringComparison) overload.

@shanselman
Copy link
Author

I love the possible case-insensitive string operators. Until then, let's do
EqualsIgnoreCase.

On Fri, Feb 6, 2015 at 10:06 AM, Eric Bickle notifications@github.com
wrote:

@davkean https://github.com/davkean @shanselman
https://github.com/shanselman Agree on EqualsIgnoreCase.

public bool EqualsIgnoreCase(string value);
public static bool EqualsIgnoreCase(string a, string b);

I did a quick scan through the String class, other methods that take a
StringComparison overload include Compare, CompareOrdinal, CompareTo,
StartsWith, EndsWith, IndexOf, LastIndexOf, Replace. None of them seem
like a high priority for an IgnoreCase overload.

This may be a bit more of a stretch, a new IStringEquatable interface
could be introduced that inherits from IEquatable. IStringEquatable would
add the necessary overloads for the comparison type.

public IStringEquatable : IEquatable
{
bool Equals(T other, StringComparison comparisonType);
}

The C# team is currently looking at potentially implementing pattern
matching and improvements to the switch statement for C# 7.0. One source of
pain has been the inability to do case insensitive comparisons in a switch
statement - introducing the interface would provide a hook for case
insensitive comparisons as 'syntactic sugar'. The interface could be added
to the other string-like types, such as Char, StringBuilder, and HtmlString.

I never quite understood why languages don't introduce a case-insensitive
string comparison operator. It's likely that because it only applies to a
string the logic is that it doesn't meet the broad use needed for a general
language feature. That said, the same was once true for String types
themselves (just use an array!). Case-insensitive string comparisons are a
very bread & butter development operation, and as noted above can apply to
types other than System.String.

Imagine the theoretical ==~ and !=~ operators:

public static String overload ==(String a, String b)
public static String overload !=
(String a, String b)

if ("CoreFx" ==~ "corefx") // true

Even aside from theoretical language changes (very unlikely), the
interface would allow libraries such as LINQ to leverage case-insensitive
string equality natively without resorting to reflection. The same holds
true for String.Compare, which strangely is missing a non-static String.Compare(string,
StringComparison) overload.


Reply to this email directly or view it on GitHub
https://github.com/dotnet/corefx/issues/649#issuecomment-73284160.

Scott Hanselman
Donate to fight diabetes: http://hnsl.mn/fightdiabetes

@pdelvo
Copy link

pdelvo commented Feb 6, 2015

I don't like the operator. It does not really self explain what it does because "approximate equals" is not well defined and could mean many different things. Is "Hello" approximate equals to "Hello!"? The Levensthein distance is 1 so it is approximatly the same, isn't it? If it is implemented for string it will probably be available to be implemented for other types. If I implement a numeric type. is 1 ==~ 0? If im looking at numbers between 0 and 1 its really not. If I am looking at two numbers a and b which are ~10^100 and (a - b) == 1 it is approximatly 0 isnt it? Other than that if(a ==b) could mean if( a == b) and if(a == (b)) so the syntax would not work either. (== would work)

@Inverness
Copy link

I don't like the operators myself for the reason mentioned, it isn't something that would be broadly used.

As someone who once used Java before moving to C#, I always create an EqualsIgnoreCase() extension method as my shorthand. I think that is the best option here along with a static version.

@omariom
Copy link
Contributor

omariom commented Feb 8, 2015

Just found this in CoreClr's sstring.h :)

BOOL EqualsCaseInsensitive(const SString &s) 

@jaredpar
Copy link
Member

jaredpar commented Feb 9, 2015

Should we consider both an instance and static method EqualsIgnoreCase?

The instance method is great for cases outlined in @shanselman example when one string is guaranteed to be non null. In more general cases though this isn't true and developers have to write out the more elaborated form:

if (sort != null && sort.EqualsIgnoreCase("Title"))

A static method which considers null for both arguments would be a bit more concise:

if (string.EqualsIgnoreCase(sort, "Title"))

The string type already has both instance and static methods for Equals so it would be building off of that existing pattern.

@shanselman
Copy link
Author

Can't folks do this now? That handles the foo is null situation.

foo?.EqualsIgnoreCase("Whatever")

On Mon, Feb 9, 2015 at 7:57 AM, Jared Parsons notifications@github.com
wrote:

Should we consider both an instance and static method EqualsIgnoreCase?

The instance method is great for cases outlined in @shanselman
https://github.com/shanselman example when one string is guaranteed to
be non null. In more general cases though this isn't true and developers
have to write out the more elaborated form:

if (sort != null && sort.EqualsIgnoreCase("Title")

A static method which considers null for both arguments would be a bit
more concise:

if (string.EqualsIgnoreCase(sort, "Title"))

The string type already has both instance and static methods for Equals
so it would be building off of that existing pattern.


Reply to this email directly or view it on GitHub
https://github.com/dotnet/corefx/issues/649#issuecomment-73533033.

Scott Hanselman
Donate to fight diabetes: http://hnsl.mn/fightdiabetes

@jaredpar
Copy link
Member

jaredpar commented Feb 9, 2015

@shanselman

That won't work as typed because it produces a bool? instead of a bool. It would need another step to convert the value to a bool

if ((foo?.EqualsIgnoreCase("Whatever")).GetValueOrDefault())

Note: the extra () around the foo?.EqualsIgnoreCase are necessary.

@ellismg
Copy link
Contributor

ellismg commented Feb 9, 2015

I think the other problem with that sample is that (foo?.EqualsIgnoreCase(bar).GetValueOrDefault()) where foo and bar are both null would produce false, whereas you might expect true to be produced (like the static string.Equals method does).

@nvivo
Copy link

nvivo commented Mar 7, 2015

I would vote for String.Equals(string other, bool ignoreCase).

This is already used in a lot of methods in the String class. If "typing less" is the idea, this would match the 80% rule.

Honestly, EqualsIgnoreCase is very strange. That opens questions to all possible alternatives. Why not ContanisIgnoreCase, EndsWithIgnoreCase, etc. Adding a new operator adds even more questions.

What I would like to see even more is that all methods in the String class follow the same pattern.

@nvivo
Copy link

nvivo commented Mar 7, 2015

After reading more in this discussion, I'm amazed how this change, that if done like proposed is one of a kind and adds a single different method to a core API just to save some typing, is being accepted like this. Many other simple issues are suffering to pass the "won't change core types like that" review from the team.

I'm just a regular guy here and I'm not sure if this was really accepted, but I see a lot of microsoft guys agreeing on this, so that is what it looks like.

Now, I think System.String needs some improvements just like the next guy, but it seems this should be a more planned change that includes more things in a standard way for such a core type.

See, the 4th most voted question for C# stackoverflow is about the same problem in String.Contains, with 1079 votes. The same question for String.Equals has only 73 votes.

And you are already proposing adding new operators to C# just for that. That's crazy! =)

Shouldn't we standardize all methods to accept a StringComparison and a bool ignoreCase before thinking about adding new methods as shortcuts?

@KrzysztofCwalina
Copy link
Member

This thread touches on several important problems in our APIs. I think we should try to address these. But I thing we need a wholistic proposal, not just add an overload here or there.

@joshfree joshfree assigned KrzysztofCwalina and unassigned ellismg Oct 12, 2015
@terrajobst
Copy link
Member

Let's not hold this API addition hostage to all the other issues string has. So far, I'm aware of two different, but related issues:

  1. Simplified equality check

     namespace System {
         public sealed class String {
    +        public bool EqualsIgnoreCase(string value);
    +        public static bool EqualsIgnoreCase(string a, string b);
         }
     }
  2. Contains with StringComparsion

      namespace System {
         public sealed class String {
    +        public bool Contains(string value, StringComparsion comparisonType);
         }
     }

@terrajobst
Copy link
Member

@weshaggard @KrzysztofCwalina Any pushback on the proposal for EqualsIgnoreCase?

@omariom
Copy link
Contributor

omariom commented Jan 5, 2016

And don't forget pls about related Slice[char] operations.

@nvivo
Copy link

nvivo commented Jan 5, 2016

Am I the only one who thinks str.EqualsIgnoreCase(...) is a bad idea?

Sounds like this opens the door for adding lots of *IgnoreCase methods for other cases. Some methods like Compare, StartsWith and EndsWith already have a pattern of using bool ignoreCase.

So, why not String.Equals(string other, bool ignoreCase)?

I'm being picky, but I'd like to see less patterns appear just for personal preferences. Much better to just follow what is already there.

@KrzysztofCwalina
Copy link
Member

@nvivo, I am not sure if it's bad or not, but I think we need to be super careful adding APIs to such basic types like string. We already have so many options here (comparers, interfaces, delegates, supporting enums, etc.). This is why I said I think we need a holistic approach, i.e. a proposal listing problems with comparison APIs in BCL, paint picture how we would like the APIs to look in the future, and an engineering plan to get us there.

@terrajobst, normally I would agree with you, but not in case of System.String. I really don't think we can add APIs to string without proper design/spec (like in the old days)

@dotnetchris
Copy link

Definitely should have operator level support for OrdinalIgnoreCase, i can already write extension methods. We don't need a BCL method, we need an operator

@DmytroSokhach
Copy link

DmytroSokhach commented Jun 7, 2016

In my case I do not actually need an operator. I would like to write simple switch for every case I get. But now I end up with multi-chain if...else if... else if... else...if...etc.
I wish to be able have 1 line conversion to the simple string-like class with OrdinalIgnoreCase comparison behavior.
So I would probably make own class with compare operator override and replace all those non maintainable chains in code.

@dotnetchris
Copy link

dotnetchris commented Jun 7, 2016

@DmitriySokhach that sounds alot like pattern matching. https://github.com/YangFan789/PatternMatchingExtension might be relevant to you

@HppZ
Copy link

HppZ commented Jun 9, 2016

+1

@Rab815
Copy link

Rab815 commented Jul 1, 2016

@shanselman and @terrajobst What was ever the outcome of this... we are doing this a lot in our projects
string.Equals("String1","String1", StringComparison.OrdinalIgnoreCase)

was an extension ever entered in 4.6 or C# 6 feature set. Or are people still creating their own extension methods or operators?

@KrzysztofCwalina
Copy link
Member

@DmitriySokhach, you could do this:

struct OrdinalIgnoreCaseString : IEquatable<OrdinalIgnoreCaseString>, IEquatable<string> {
    string _str;
    public OrdinalIgnoreCaseString(string str) {
        _str = str;
    }

    public static implicit operator OrdinalIgnoreCaseString(string str) {
        return new OrdinalIgnoreCaseString(str);
    }

    public static explicit operator string(OrdinalIgnoreCaseString str) {
        return str._str;
    }

    public bool Equals(string other) {
        return _str.Equals(other, StringComparison.OrdinalIgnoreCase);
    }

    public bool Equals(OrdinalIgnoreCaseString other) {
        return _str.Equals(other._str, StringComparison.OrdinalIgnoreCase);
    }
}

@dotnetchris
Copy link

@KrzysztofCwalina i created this on a gist since this seems like something worth saving! https://gist.github.com/dotnetchris/75f2cf6137b8e0e37961a3301e1dad5b

@KrzysztofCwalina
Copy link
Member

@dotnetchris, great; thanks!

@terrajobst
Copy link
Member

We currently don't have a proposal to review, thus I've marked it as api-needs-work. We'll take a look whether someone on our side will do this.

@joperezr
Copy link
Member

Marking as up-for-grabs so that anybody can submit an API Proposal to look at.

@andersstorhaug
Copy link

Something that makes this a little bit less painful with C# 6:

using static System.StringComparer;
if (OrdinalIgnoreCase.Equals(alpha, beta))
{ 
    // ...
}

@remcolam
Copy link

My personal solution would be to throw our the StringComparison enum completely, and replace all its uses with the StringComparer static instances. All methods that take StringComparison can easily be rewritten using this.

The big issue is that you always need a second to thing about which one of the two you need. The typing is not the problem, that's what intellisense/autocomplete is for

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Aug 20, 2019

Summarising all the ideas & slipping my cent in :^)

I think it's fair to provide static versions of the methods as well since we have them for all the other overloads/methods.

1. Providing an overload of Equals(string) that takes an additional boolean parameter ignoreCase

Suggested API addition

public sealed class String
{
    public string Equals(string str, bool ignoreCase);
    public static string Equals(string a, string b, bool ignoreCase);
}

Pros

  • Matches what other methods do, such as IndexOf, Contains, EndsWith, Compare etc

Cons

  • Overloads of other methods that accept bool does not indicate culture insensitive comparison, but only case sensitivity... though, it might be fine in this instance since string.Equals is ordinal by default.

Usage

if (str1.Equals(str2, true))
{
    // The two strings are considered equal, disregarding culture/case
}

2. Providing a new method EqualsIgnoreCase(string)

Suggested API addition

public sealed class String
{
    public string EqualsIgnoreCase(string value);
    public static string EqualsIgnoreCase(string a, string b);
}

Pros

  • Maybe... shorter to type?

Cons

  • Same as the disadvantage described in Issue - corefx#1; implied culture insensitivity.
  • Missing the matching culture sensitive alternative, What if I want to do culture sensitive / case insensitive comparison?

Usage

if (str1.EqualsIgnoreCase(str2))
{
    // ...
}

3. Providing a new method EqualsOrdinalIgnoreCase(string)

Suggested API addition

public sealed class String
{
    public string EqualsOrdinalIgnoreCase(string value);
    public static string EqualsOrdinalIgnoreCase(string a, string b);
}

Pros

  • Being explicit about both the culture/case insensitivity

Cons

  • About as long to type

Usage

if (str1.EqualsOrdinalIgnoreCase(str2))
{
// ...
}

4. Providing a new method EqualsOrdinal that takes a string, and an overload that takes an additional parameter ignoreCase. Also an additional Equals(string, bool) overload

Suggested API addition

public sealed class String
{
    public string Equals(string value, bool ignoreCase);
    public static string Equals(string a, string b, bool ignoreCase);
    public string EqualsOrdinal(string value);
    public string EqualsOrdinal(string value, bool ignoreCase);
    public static string EqualsOrdinal(string a, string b);
    public static string EqualsOrdinal(string a, string b, bool ignoreCase);
}

(Maybe ignoreCase can be optional?)

Pros

~~+ Matches what other methods do, by providing a bool ignoreCase parameter ~~
+ Matches how Compare has a separate method group dedicated for culture insensitive comparison (Compare and CompareOrdinal, although CompareOrdinal has no instance methods)
+ Explicit about culture insensitivity, which solves the disadvantage described above
+ Also provides easy culture insensitive/case sensitive comparison

Cons

- To be discussed

Usage

if (str1.EqualsOrdinal(str2, true))
{
    // The two strings are considered equal, disregarding culture/case
}

Apparently I was thinking that string.Equals(string) defaults to current culture when its not. 😅

Existing alternatives

1. using the current string.Equals(string, StringComparison)

if (str1.Equals(str2, StringComparison.OrdinalIgnoreCase))
{
    // ...
}

2. Using StringComparer (Suggested by @\andersstorhaug)

using static System.StringComparer;
if (OrdinalIgnoreCase.Equals(alpha, beta))
{ 
    // ...
}

@iSazonov
Copy link
Contributor

Adding EqualsIgnoreCase() will add more complicity and open a way for bugs I believe.
In PowerShell repo we came to always use explicit values for string comparisons. PowerShell is almost always case-insensitive but not always. It uses currentculture for console output but invariant culture or sometimes ordinal in engine. Therefore, we must be careful and always use explicit values so that a code read will always see design intentions. I think it is true for every considerable application.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@joperezr joperezr modified the milestones: 5.0.0, Future Jul 6, 2020
@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2020
@GrabYourPitchforks
Copy link
Member

This issue is going on six years old and has not had any activity in over a year. We're clearly not going to change the behavior of the existing string.Equals or string.operator== APIs.

The proposal that got the most upvotes in this issue was introducing an EqualsIgnoreCase API, but even that spawned a bit of disagreement.

There are allusions to more fundamental problems with the string APIs and equality checking, but as far as I can tell nobody in this thread has articulated them. If somebody wants to take a stab at clearly explaining the problem, that would help us properly address this issue.

Is the problem that we'd prefer the default equality check be ordinal case-insensitive instead of ordinal case-sensitive? (This isn't going to happen.) Is the problem that other string APIs are culture-aware by default? (That's not really relevant to an accelerator for EqualsIgnoreCase.) Is the problem that this is just too much typing and there needs to be an accelerator? (We could address that at an API level or at a language level.)

As an example of a language accelerator that requires no API additions, there's always:

using static System.StringComparison;

string candidate = ...;
if (candidate.Equals("hello", OrdinalIgnoreCase)) { /* do something */ }

If the goal is to avoid writing the literal text StringComparison more than once per file, does this solve the issue? Without a clearly articulated problem statement, this issue is not currently actionable and is likely to be closed.

@divega
Copy link
Contributor

divega commented Sep 30, 2021

While I see how adding a case-insensitive version of Equals or an accelerator would help reduce friction in some cases, I am more interested in a solution that allows developers to control the behavior of regular Equals. For example, being able to make the Equals and GetHashCode generated for record types case-insensitive for string members would remove a lot of friction in the projects I am currently working on (control plane of Azure services). Without this capability, records are useless to me.

If a case-insensitive version of String (say OrdinalIgnoreCaseString) was available, I could use that.

Another option I can imagine is to extend String to allow specifying at creation time what flavor of string comparison to employ with it by default. Although this sounds more flexible, I am sure it will be more verbose and it might not be sufficient: When a string instance is assigned to the string properties of my records I want any further calls to Equal to be case insensitive, no matter the origin of the string. If case sensitive and case insensitive strings were represented by the same type, there would be a simple assignment and I wouldn't get the behavior I want (I will still get the StringComparsion that was specified when the string was created). I need the types to be different, to force a conversion before the assignment.

@GrabYourPitchforks do you know if there is another issue more in line with what I want?

@molinch
Copy link

molinch commented Nov 3, 2022

Finding a consensus here might be pretty hard.
Why not going a completely different direction and having a project level switch? For example:
<DefaultStringComparison>Ordinal</DefaultStringComparison>

This would allow people to opt-in to their desired comparer really per project.
In our projects we keep speficying the comparer absolutely ALL the time, and if we forget the proper comparer we introduce a bug.

For sure analyzers can solve this, but having to type extra characters all the time to get the proper behavior is just a pain (especially when that behavior should be the default). Furthermore as @divega said, without a solution records are just useless in many scenarios.

Implementation wise, Roslyn could probably do the heavy lifting and inject the comparer in all ctors/methods that expect a StringComparison/StringComparer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Projects
None yet
Development

No branches or pull requests