12
\$\begingroup\$

I have a method which I use on occasion to convert strings (like enum and property names) to PascalCase for display.

It's fairly simple, so there's probably not much to comment on, but as usual I appreciate any advice on improving it.

/// <summary>
/// Converts a string of dash-separated, or underscore-separated words to a PascalCase string.
/// </summary>
/// <param name="s">The string to convert.</param>
/// <returns>The resulting PascalCase string.</returns>
public static string ToPascalCase(this string s)
{
    string[] words = s.Split(new char[2] { '-', '_' }, StringSplitOptions.RemoveEmptyEntries);

    StringBuilder sb = new StringBuilder(words.Sum(x => x.Length));

    foreach (string word in words)
    {
        sb.Append(word[0].ToString().ToUpper() + word.Substring(1));
    }

    return sb.ToString();
}

Feel free to use it if you like, as per my usual regards.

\$\endgroup\$
  • 3
    \$\begingroup\$ Curious observation: you force the first character of a word to be uppercase but don't force subsequent characters to be lowercase. So "FOO-BAR" will return "FOOBAR" and not "FooBar". \$\endgroup\$ – Rick Davin Dec 3 '15 at 21:28
  • \$\begingroup\$ @RickDavin Very good point, I'll have to make that correction. \$\endgroup\$ – Der Kommissar Dec 3 '15 at 21:28
8
\$\begingroup\$

Doing string concatenation to pass a string argument to the Append () method of the StringBuilder is the wrong way. Use instead multiple calls to prevent unneccessary creation of string objects. There are overloads for the different types so you could call it once for the starting uppercase char and another which takes the remaining string.

If by any chance there are some localized strings involved where the unicode textlength isn't 1 for a character, you won't get the expected results. See the accepted answer and mine here

While initializing a StringBuilder with a capacity is the way to go, I would like to suggest to just use the Length of the passed string s.

The passed in string s should be checked against null and if it is null throw an ArgumentNullException which would be more correct than the NullReferenceException you would get without the check.

| improve this answer | |
\$\endgroup\$
11
\$\begingroup\$

I think you should use Concat instead of StringBuilder:

public static string ToPascalCase(this string s)
{
    var words = s.Split(new[] { '-', '_' }, StringSplitOptions.RemoveEmptyEntries)
                 .Select(word => word.Substring(0, 1).ToUpper() + 
                                 word.Substring(1).ToLower());

    var result = String.Concat(words);
    return result;
}

This solution should not be as fast as manually looping through the original string, but I think it is much easier to read and follow. Unless you do those convertions like 1000000 times per second, you should not complicate things.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I thought source code in an amswer on codereview would be more readable than this :) Formatting it to fit to the readable area at least would be nice. \$\endgroup\$ – Oguz Ozgul Dec 4 '15 at 6:43
  • 1
    \$\begingroup\$ @OguzOzgul fair enough. \$\endgroup\$ – Nikita B Dec 4 '15 at 6:50
  • 2
    \$\begingroup\$ This is readable, good! Instead of string.Join with string.Empty, use string.Concat. Also do not mix String and string. I believe best practice is to use string in C#. \$\endgroup\$ – Oskar Sjöberg Dec 4 '15 at 10:16
  • \$\begingroup\$ Good idea about Concat! I use String, when I access static members and string when declaring variables. I cant really explain why, but I am used to it. \$\endgroup\$ – Nikita B Dec 4 '15 at 10:38
8
\$\begingroup\$

Compared to the String, StringBuilder is mutable. Meaning you don't really need to Append multiple times, you could just replace the characters from the original string:

//that's just for the test
string s = "-asd_asd_asd-asd-";

if(s.Length == 0)
{
    return s;
}

var sb = new StringBuilder(s.ToLowerInvariant());
sb[0] = Char.ToUpper(sb[0]);

for (int i = 0; i < sb.Length; i++)
{
    if (sb[i] == '_' || sb[i] == '-')
    {
        sb.Remove(i, 1);
        if (i != sb.Length)
        {
            sb[i] = Char.ToUpper(sb[i]);
        }
    }
}

Using this technique you don't need to spit the string and do substrings. Obviously, there're more if involved :p But I think it would be faster.

| improve this answer | |
\$\endgroup\$
7
\$\begingroup\$

If performance is your main concern, you might want to loop through the string just once, using a for loop, and appending substrings to the StringBuilder.

It will make your code a little longer and mebe a little harder to read.

Here is an example of what it might look like.

public static string ToPascalCase(this string s)
{
    StringBuilder sb = new StringBuilder(s.Length);
    int a = 1, b = 1;
    bool toUpper = true;

    for (int i = 0; i < s.Length; i++)
    {
        switch (s[i])
        {
            case '-':
            case '_':
                if (a != b)
                {
                    sb.Append(s.Substring(a, b - a));
                }
                a = b = i + 2;
                toUpper = true;
                break;

            default:
                if (toUpper)
                {
                    sb.Append(char.ToUpper(s[i]));
                    toUpper = false;
                }
                else
                {
                    b++;
                }
                break;
        }
    }

    if (a != b)
    {
        sb.Append(s.Substring(a, b - a));
    }

    return sb.ToString();
}

But as far as your code goes, it's short, readable, and unless you are performing millions of these all the time, I think your version is more than adequate.

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

This is my take on it, the akward part is that you need to convert the string to lower case before calling ToTitleCase because if all letters in a word are uppercase it will remain uppercased as it is identified as an acronym and preserved by the ToTitleCase implementation.

public static string ToPascalCase(this string text)
{
    return CultureInfo.InvariantCulture.TextInfo
        .ToTitleCase(text.ToLowerInvariant())
        .Replace("-", "")
        .Replace("_", "");
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I have thought about ToTitleCase() as well, but if you consider that each char which follows any non alphanumeric character will be titlecased, this won't produce the same result. Nevertheless +1 at least for using ToLowerInvariant() \$\endgroup\$ – Heslacher Dec 4 '15 at 10:48
2
\$\begingroup\$

This version has got more readability using RegexReplace:

public static string ToPascalCase(this string text)
{
    const string pattern = @"(-|_)\w{1}|^\w";
    return Regex.Replace(text, pattern, match => match.Value.Replace("-", string.Empty).Replace("_", string.Empty).ToUpper());
}
| improve this answer | |
\$\endgroup\$
  • 6
    \$\begingroup\$ "Has got more readability using Regex" is oxymoron. :) \$\endgroup\$ – Nikita B Dec 4 '15 at 9:42
  • \$\begingroup\$ Apart from Regex pattern, I feel the code is bit cleaner. \$\endgroup\$ – SiD Dec 4 '15 at 10:02
  • \$\begingroup\$ Its just that in my experience most people do not understand regex syntax off the top of their heads. I know I don't, I have to google it every time. :) Thats why I am sceptical about Regex solution being "more readable". I might be wrong though. \$\endgroup\$ – Nikita B Dec 4 '15 at 10:11
  • \$\begingroup\$ You have missed the possbility that there are two _'s in a row and you might want to use groups instead of the replaces. Also the \w will match _'s so that might cause some wierd behaviour. const string pattern = @"(?<fc>[^\W_])|[-_]+(?<fc>[^\W_]{0,1})"; Console.WriteLine(Regex.Replace(text, pattern, m => m.Groups["fc"].Value.ToUpper())); \$\endgroup\$ – sQuir3l Dec 4 '15 at 16:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.