Analyzer proposal: Consider using String.Equals instead of String.Compare

[xtqqczze]

Category: Microsoft.Usage

Fix is breaking or non-breaking: Non-breaking

Cause

This rule locates calls to Compare where the result is used to check for equality, and suggests using Equals instead, to improve readability.

Rule description

When Compare is used to check if the result is equal to 0, the call can be safely substituted with Equals.

Overload	Suggested fix
String.Compare(string)	String.Equals(string)
String.Compare(string, false)	String.Equals(string, StringComparison.CurrentCulture)
String.Compare(string, true)	String.Equals(string, StringComparison.CurrentCultureIgnoreCase)
String.Compare(string, StringComparison)	String.Equals(string, StringComparison)

Examples

Code with Diagnostic

string.Compare(x, y) == 0
string.Compare(x, y) != 0
string.Compare(x, y, false) == 0
string.Compare(x, y, true) == 0
string.Compare(x, y, StringComparison.CurrentCulture) == 0

Code with Fix

string.Equals(x, y)
!string.Equals(x, y)
string.Equals(x, y, StringComparison.CurrentCulture)
string.Equals(x, y, StringComparison.CurrentCultureIgnoreCase)
string.Equals(x, y, StringComparison.CurrentCulture)

When to suppress warnings

It's safe to suppress a violation of this rule if improving code readability is not a concern.

Additional context

• Inspiration came from RCS1235: Optimize method call.
• Proposal was modelled on documentation for CA2249: Consider using String.Contains instead of String.IndexOf.

[Evangelink]

@mavasani not sure who could confirm there is indeed a performance issue.

[xtqqczze]

Perhaps this a more of a code style issue?

[Therzok]

Compare and Equals

From the looks of it, between the two, Equals fast-paths on a .Length check, Compare does a full string comparison.

[mavasani]

This is a .NET API usage analyzer suggestion. @jeffhandley @carlossanlop @buyaa-n can this please be ported to dotnet/runtime repo and triaged?

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Code with Diagnostic

string.Compare(x, y, StringComparison.CurrentCulture) == 0

Code with Fix

string.Equals(x, y, StringComparison.CurrentCulture)

Additional context

See RCS1235

Author:	xtqqczze
Assignees:	-
Labels:	area-System.Globalization, untriaged
Milestone:	-

[tarekgh]

I am not sure why we need this? can you explain more why we need that? both are valid options and both are preferable as long as providing the StringComparison options. I don't think we need that in the analyzers.

[buyaa-n]

I am not sure why we need this? can you explain more why we need that? both are valid options and both are preferable as long as providing the StringComparison options. I don't think we need that in the analyzers.

Sure, both are valid options, as @Therzok mentioned above we expect it will give a bit of performance benefit. From that point isn't string.Equals(x, y, StringComparison.CurrentCulture) is preferable than string.Compare(x, y, StringComparison.CurrentCulture) == 0? What you think @tarekgh