Skip to content

Conversation

goran-w
Copy link
Contributor

@goran-w goran-w commented Aug 12, 2025

Changed the comparison method used in NumericSort.Compare() :

  • Ordinal comparison sorts non-alphanumeric chars (like underscore etc) differently than expected (i.e compared to filesystem sorting etc).
  • InvariantCulture comparison corrects this, without imposing cultural differences.
  • (A possible alternative would be CurrentCulture comparison.)

Additionally:

  • When comparing a single digit-char against a single non-digit-char, the earlier code used char.CompareTo() where the comparison method can not be specified.
  • To fix this, we now use the same consistent Comparer object for all the string comparisons which are NOT "digits vs digits".

@goran-w goran-w changed the title Make NumericSort use InvariantCulture instead of Ordinal comparison Make NumericSort use InvariantCulture for text comparison Aug 13, 2025
@goran-w
Copy link
Contributor Author

goran-w commented Aug 18, 2025

@love-linger NOTE: This PR does NOT change the behavior of numeric comparisons in the NumericSort class.

My change only improves how non-numeric comparisons are made (to handle symbols like underscore in a way that's more consistent with the filesystem etc).

Also, it fixes an actual bug where the single-char (digit vs non-digit) case used a different comparison method than the normal (non-numeric) substring case!

Finally, by using a comparator object for the non-numeric comparisons, it becomes very easy to change the comparison-method used (if needed).

@love-linger
Copy link
Collaborator

https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings

image

I want to write a relevant performance test result, but I don't have time recently.

@goran-w goran-w force-pushed the more_accurate_sorting branch from ff9e488 to 1b81d86 Compare August 18, 2025 15:42
@goran-w
Copy link
Contributor Author

goran-w commented Aug 18, 2025

An alternative could be to use CurrentCulture : "when you display output to the user", as mentioned in the documentation section you quoted... (I was trying to use something that was closer to Ordinal, i.e culture-agnostic, but still did something more "intelligent" than sorting chars by their underlying char-code like Ordinal seems to do.)

For example, when sorting with OrdinalIgnoreCase, names beginning with underscore are sorted at the bottom rather than at the top like in the filesystem (Windows Explorer etc). Using either InvariantCultureIgnoreCase or CurrentCultureIgnoreCase fixes this.

Example of sort-order with OrdinalIgnoreCase :
image

Example of sort-order with InvariantCultureIgnoreCase or CurrentCultureIgnoreCase (and in filesystem listings etc) :
image

BTW, I don't think this has a noticeable effect on performance - doing the "Numeric Sorting" (at all) probably has a larger impact anyway?

@love-linger love-linger force-pushed the develop branch 5 times, most recently from f325aaf to 6511d15 Compare August 19, 2025 07:54
Ordinal comparison sorts non-alphanumeric chars (like underscore etc) differently than expected (i.e compared to filesystem sorting etc).
InvariantCulture comparison corrects this, without imposing cultural differences. (A possible alternative would be CurrentCulture comparison.)
The earlier code, when comparing a single digit-char against non-digit-char, used char.CompareTo() where the comparison method can not be specified.
To fix this, we now use the same Comparer for all non-all-digits string comparisons.
@goran-w goran-w force-pushed the more_accurate_sorting branch from 1b81d86 to 87714a7 Compare August 19, 2025 08:46
@love-linger love-linger self-assigned this Aug 25, 2025
@love-linger love-linger added the enhancement New feature or request label Aug 25, 2025
@love-linger love-linger merged commit a916403 into sourcegit-scm:develop Aug 25, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants