Skip to content

Adjust ToCamelCase Extension #8274

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

Closed
wants to merge 1 commit into from

Conversation

cwuethrich
Copy link

Adjust ToCamelCase Extension of string to match the behavior of System.Text.Json and Newtonsoft.Json.
Bug: #8273

{
var current = chars[i];

if (char.IsSeparator(current))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (char.IsSeparator(current))
if (!char.IsLetter(current))

Copy link
Member

@flobernd flobernd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR 🙂

I suggested a few adjustments.

One suggestion is a minor performance optimization using a stack allocated Span instead of allocating a new array on the heap.

The other suggestion is a functional one. I think we want to handle e.g. digits the same way as separators so that for instance SHA1 is converted to sha1 instead of shA1. To save some checks, I reversed the logic to check for char.IsLetter() (or !char.IsLetter()) instead of explicitly excluding separators, digits, etc.

Edit:
In this usecase, we don't expect to ever encounter "whitespace" characters as they are invalid to use in .NET field/property-names, but if we want to be even more accurate, we would have to add special handling for separators like e.g. _. For example Camel_Case should end up as camelCase as camel-case does not allow such separators.

if (0 < i && i + 1 < chars.Length)
{
var next = chars[i + 1];
if (!char.IsUpper(next) && !char.IsSeparator(next))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!char.IsUpper(next) && !char.IsSeparator(next))
if (char.IsLetter(next) && !char.IsUpper(next))

var camelCase = char.ToLowerInvariant(s[0]).ToString();
if (s.Length > 1)
camelCase += s.Substring(1);
var chars = s.ToCharArray();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var chars = s.ToCharArray();
#if NET6_0_OR_GREATER
Span<char> chars = stackalloc char[s.Length];
s.CopyTo(chars);
#else
var chars = s.ToCharArray();
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires an additional:

#if NET6_0_OR_GREATER
using System;
#endif

@flobernd
Copy link
Member

Actually, let's just copy the System.Text.Json implementation (MIT license):

    internal static string ToCamelCase3(this string name)
    {
        if (string.IsNullOrEmpty(name) || !char.IsUpper(name[0]))
        {
            return name;
        }

#if NET
        return string.Create(name.Length, name, (chars, name) =>
        {
            name.CopyTo(chars);
            FixCasing(chars);
        });
#else
            char[] chars = name.ToCharArray();
            FixCasing(chars);
            return new string(chars);
#endif
    }

    private static void FixCasing(Span<char> chars)
    {
        for (var i = 0; i < chars.Length; i++)
        {
            if (i == 1 && !char.IsUpper(chars[i]))
            {
                break;
            }

            var hasNext = (i + 1 < chars.Length);

            // Stop when next char is already lowercase.
            if (i > 0 && hasNext && !char.IsUpper(chars[i + 1]))
            {
                // If the next char is a space, lowercase current char before exiting.
                if (chars[i + 1] == ' ')
                {
                    chars[i] = char.ToLowerInvariant(chars[i]);
                }

                break;
            }

            chars[i] = char.ToLowerInvariant(chars[i]);
        }
    }

@cwuethrich
Copy link
Author

cwuethrich commented Jul 29, 2024

Wasn't sure about the license. But if we are allowed to use it, it would be the best. Then the properties for sure gonna match. Sad they didn't make theirs classes public.

Edit: Do you integrate it, or should I go for a try?

@flobernd
Copy link
Member

@cwuethrich I can open a new PR, if you don't mind 🙂 Thanks for creating this one!

@flobernd flobernd closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Relates to a 8.x client version Area: Client Category: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants