Name proper casing

Question

I deal with hundreds and hundreds of names each week. These names (along with other pedigree information) are stored in a database. Typically, I get these names in all sorts of formats, mainly proper and in all-caps. I needed an easy way to convert the names (especially last names) to their proper format:

• McDonald
• MacDougal
• Smith-Jones
• Davis II
• George IV

The following code properly formats the above examples as well as standard names. Please take a look at my code and suggest areas where I might make it more efficient. I got the idea for this method mainly from this link.

public string ConvertToProperNameCase(string input)
{
    bool SuffixProcessed = false;
    input = input.Trim();

    if (String.IsNullOrEmpty(input))
    {
        return String.Empty;
    }

    char[] chars = CultureInfo.CurrentCulture.TextInfo.ToTitleCase(input.ToLower()).ToCharArray();

    for (int i = 0; i + 1 < chars.Length; i++)
    {
        if ((chars[i].Equals('\'')) ||
            (chars[i].Equals('-')))
        {
            chars[i + 1] = Char.ToUpper(chars[i + 1]);
        }
    }

    string s = new string(chars);

    if (((s.ToUpper().StartsWith("MAC") || s.ToUpper().StartsWith("MCC") || s.ToUpper().StartsWith("DE ")) && s.Length > 4))
    {
        try
        {
            s = s.Substring(0, 1).ToUpper() + s.Substring(1, 2).ToLower() + s.Substring(3, 1).ToUpper() + s.Substring(4).ToLower();
        }
        catch
        {
            s = s;
        }
    }

    if ((s.ToUpper().StartsWith("MC")) && s.Length > 3)
    {
        try
        {
            s = s.Substring(0, 1).ToUpper() + s.Substring(1, 1).ToLower() + s.Substring(2, 1).ToUpper() + s.Substring(3).ToLower();
        }
        catch
        {
            s = s;
        }
    }

    if (s.ToUpper().Contains(" III") && !SuffixProcessed)
    {
        try
        {
            s = s.Substring(0, s.ToUpper().IndexOf(" III")) + " " + s.Substring(s.ToUpper().IndexOf(" III"), 4).ToUpper();
            SuffixProcessed = true;
        }
        catch
        {
            s = s;
        }

    }

    if (s.ToUpper().Contains(" II") && !SuffixProcessed)
    {
        try
        {
           s = s.Substring(0, s.ToUpper().IndexOf(" II")) + " " + s.Substring(s.ToUpper().IndexOf(" II"), 3).ToUpper();
           SuffixProcessed = true;
        }
        catch
        {
            s = s;
        }
    }

    if (s.ToUpper().Contains(" IV") && !SuffixProcessed)
    {
        try
        {
            s = s.Substring(0, s.ToUpper().IndexOf(" IV")) + " " + s.Substring(s.ToUpper().IndexOf(" IV"), 3).ToUpper();
            SuffixProcessed = true;
        }
        catch
        {
            s = s;
        }
    }

    return s;
}

After carefully digesting Winston Ewert's post, here is what I can up with. This code will now handle the following examples:

• MACDonald-jOnes
• FRAnK iV
• MCCarthy xiv
• mcTavish-tOMaS jr
• Robert-ROBERTSON SR

Revised code

using System;
using System.Windows.Forms;

namespace WindowsFormsApplication5
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        private void button1_Click(object sender, EventArgs e)
        {
            string[] Names = new string[]{ "MACDonald-jOnes, FRAnK iV", "MCCarthy xiv", " ", "mcTavish-tOMaS jr", "Robert-ROBERTSON SR."};

            foreach (string s in Names)
                Console.WriteLine(string.Format("Original Name: {0}\n   Converted Name: {1}", s, ConvertToProperNameCase(s)));
        }

        public string ConvertToProperNameCase(string s)
        {
            string ReturnValue = string.Empty;

            if (s.Trim().Length > 0)
            {
                try
                {
                    s = s.ToLower();

                    bool[] Name = new bool[s.Length];
                    for (int i = Name.Length; i < Name.Length; i++)
                    {
                        Name[i] = false;
                    }

                    for (int i = 0; i < s.Length; i++)
                    {
                        if (i == 0)
                        {
                            Name[i] = true;
                        }

                        if (s[i].Equals('\'') || s[i].Equals('-') || s[i].Equals(' '))
                        {
                            Name[i + 1] = true;
                        }
                    }

                    string[] Prefixes = new string[] { "MAC", "MC" };
                    bool[] b = new bool[1] { true };

                    foreach (string p in Prefixes)
                    {
                        if (s.ToUpper().Trim().StartsWith(p))
                        {
                            switch (p.Length)
                            {
                                case 2:
                                    b = new bool[] { true, false, true };
                                    break;
                                case 3:
                                    b = new bool[] { true, false, false, true };
                                    break;
                            }

                            break;
                        }
                    }

                    for (int i = 0; i < b.Length; i++)
                    {
                        Name[i] = b[i];
                    }

                    string[] Suffixes = new string[] { "XXIIII", "IIIII", "VIIII", "XVIII", "XXIII", "IIII", "VIII", "XIII", "XVII", "XXII", 
                                                       "XXIV", "III", "VII", "IIX", "XII", "XIV", "XVI", "XIX", "XXV", "XXI", "II", "IV", "VI", 
                                                       "IX", "XI", "XV", "XX", "I", "V", "X" };

                    foreach (string suf in Suffixes)
                    {
                        if (s.ToUpper().Trim().EndsWith(suf) && (s.Length > (suf.Length + 1)))
                        {
                            for (int i = s.Length - 1; i > (s.Length - (suf.Length + 1)); i--)
                            {
                                Name[i] = true;
                            }

                            break;
                        }
                    }

                    for (int i = 0; i < s.Length; i++)
                    {
                        if (Name[i] == true)
                        {
                            ReturnValue += s.Substring(i, 1).ToUpper();
                        }
                        else
                        {
                            ReturnValue += s.Substring(i, 1);
                        }

                    }

                    return ReturnValue;
                }
                catch(Exception ex)
                {
                    MessageBox.Show(ex.Message);
                    return ReturnValue = "Error";
                }
            }
            else
            {
                return ReturnValue = "Empty String";
            }
        }
    }
}

Answer 1

public string ConvertToProperNameCase(string input)
{
    bool SuffixProcessed = false;

You don't use this variable for a long time, move it closer to where it is used.

    input = input.Trim();

    if (String.IsNullOrEmpty(input))
    {
        return String.Empty;
    }

    char[] chars = CultureInfo.CurrentCulture.TextInfo.ToTitleCase(input.ToLower()).ToCharArray();

Rather then Title casing now, why don't you make everything lowercase here, selectively uppercase pieces and then have the whole thing TitleCased? That you can assume everything is lowercase throughout this function which will simplify the code.

    for (int i = 0; i + 1 < chars.Length; i++)
    {
        if ((chars[i].Equals('\'')) ||
            (chars[i].Equals('-')))
        {
            chars[i + 1] = Char.ToUpper(chars[i + 1]);
        }
    }


    string s = new string(chars);

Why don't you create an array of bools, the same length as your string. Initially have all values set to false. Set those values to true which should be uppercased. Apply the uppercasing as the last part of the function.

    if (((s.ToUpper().StartsWith("MAC") || s.ToUpper().StartsWith("MCC") || s.ToUpper().StartsWith("DE ")) && s.Length > 4))
    {
        try
        {
            s = s.Substring(0, 1).ToUpper() + s.Substring(1, 2).ToLower() + s.Substring(3, 1).ToUpper() + s.Substring(4).ToLower();

It would be a lot simpler just to set a flag in the should_uppercase array to true.

        }
        catch

Don't just catch any error. Catch the specific type of error that will be thrown. Otherwise you might catch something you didn't mean to.

        {
            s = s;

This statement does nothing.

        }
    }

    if ((s.ToUpper().StartsWith("MC")) && s.Length > 3)
    {
        try
        {
            s = s.Substring(0, 1).ToUpper() + s.Substring(1, 1).ToLower() + s.Substring(2, 1).ToUpper() + s.Substring(3).ToLower();
        }
        catch
        {
            s = s;
        }
    }

You've done essentially the same thing twice. You should put all of these "mac, mc, de, mcc" in a constant array and then loop over it do to that logic.

    if (s.ToUpper().Contains(" III") && !SuffixProcessed)

What if " III" appears somewhere that isn't the end? Do you still want to do this?

    {
        try
        {
            s = s.Substring(0, s.ToUpper().IndexOf(" III")) + " " + s.Substring(s.ToUpper().IndexOf(" III"), 4).ToUpper();
            SuffixProcessed = true;
        }
        catch

Can this actually happen? If you don't think its going to happen don't catch it as an exception and ignore it. That'll just mask bugs.

        {
            s = s;
        }

    }

    if (s.ToUpper().Contains(" II") && !SuffixProcessed)
    {
        try
        {
           s = s.Substring(0, s.ToUpper().IndexOf(" II")) + " " + s.Substring(s.ToUpper().IndexOf(" II"), 3).ToUpper();
           SuffixProcessed = true;
        }
        catch
        {
            s = s;
        }
    }

    if (s.ToUpper().Contains(" IV") && !SuffixProcessed)
    {
        try
        {
            s = s.Substring(0, s.ToUpper().IndexOf(" IV")) + " " + s.Substring(s.ToUpper().IndexOf(" IV"), 3).ToUpper();
            SuffixProcessed = true;
        }
        catch
        {
            s = s;
        }
    }

You should at least put these roman numerals in an array so you don't have to duplicate your logic. But what if Louis XIV decides to to use your system? You might want to consider something like checking whether the last word in the name consists only of X,I,V, or something.

    return s;
}

Round 2

    public string ConvertToProperNameCase(string s)
    {
        string ReturnValue = string.Empty;

You don't use ReturnValue until way later (aside from a couple places where you shouldn't be using it) so move it where you actually need it.

        if (s.Trim().Length > 0)

You end up repeatedly trimming s. Can you just trim it once and be done with it? Do you need to preserve whitespace on either end?

        {
            try
            {
                s = s.ToLower();

                bool[] Name = new bool[s.Length];
                for (int i = Name.Length; i < Name.Length; i++)
                {
                    Name[i] = false;
                }

Name may not be the best choice, because the variable doesn't indicate that it is keeping track of cases.

                for (int i = 0; i < s.Length; i++)
                {
                    if (i == 0)
                    {
                        Name[i] = true;
                    }

Just set Name[0] = true; before entering this loop.

                    if (s[i].Equals('\'') || s[i].Equals('-') || s[i].Equals(' '))
                    {
                        Name[i + 1] = true;
                    }
                }

                string[] Prefixes = new string[] { "MAC", "MC" };
                bool[] b = new bool[1] { true };

                foreach (string p in Prefixes)
                {
                    if (s.ToUpper().Trim().StartsWith(p))

Your string is lowercased. Change your prefixes so they are lowercase and you won't need to uppercase anything.

                    {
                        switch (p.Length)
                        {
                            case 2:
                                b = new bool[] { true, false, true };
                                break;
                            case 3:
                                b = new bool[] { true, false, false, true };
                                break;
                        }

You should be able to do this without switching on the length. i.e. you should be able to write code that works for anything length.

                        break;
                    }
                }

                for (int i = 0; i < b.Length; i++)
                {
                    Name[i] = b[i];
                }

Rather then writing into another boolean array, write directly into Name[]. In particular if you find a prefix you should set Name[prefix.length] = true; Name[0] will already be true.

                string[] Suffixes = new string[] { "XXIIII", "IIIII", "VIIII", "XVIII", "XXIII", "IIII", "VIII", "XIII", "XVII", "XXII", 
                                                    "XXIV", "III", "VII", "IIX", "XII", "XIV", "XVI", "XIX", "XXV", "XXI", "II", "IV", "VI", 
                                                    "IX", "XI", "XV", "XX", "I", "V", "X" };

I don't see any particular pattern to the order here. How can you be sure you haven't missed some? If you put them in numerical order it will be more obvious. I also suggest moving the array outside of the function. I still think it would be cleaner to find the last word and figure out whether it looks like a roman numeral.

                foreach (string suf in Suffixes)
                {
                    if (s.ToUpper().Trim().EndsWith(suf) && (s.Length > (suf.Length + 1)))

No need for the parents around suf.Length+ 1

                    {
                        for (int i = s.Length - 1; i > (s.Length - (suf.Length + 1)); i--)

This may be better if you counted forwards rather then backwards.

                        {
                            Name[i] = true;
                        }

                        break;
                    }
                }

                for (int i = 0; i < s.Length; i++)
                {
                    if (Name[i] == true)
                    {
                        ReturnValue += s.Substring(i, 1).ToUpper();
                    }
                    else
                    {
                        ReturnValue += s.Substring(i, 1);
                    }

                }

It may be cleaner to convert to an array of chars, upper case elements in that array, and convert back to a string. Concatenating strings together will not be very good for performance. I think there should also be a specialized method for returning individual characters rather then fetching substrings of length 1.

                return ReturnValue;
            }
            catch(Exception ex)
            {
                MessageBox.Show(ex.Message);
                return ReturnValue = "Error";

I recommend not including GUI related logic in a function like this. This function should worry about handling the name logic and let something else worry about handling UI display.

            }
        }
        else
        {
            return ReturnValue = "Empty String";

Why are you assigning ReturnValue like that? There isn't any point since ReturnValue is a local variable that will be thrown away when the function ends. Just return the "Empty String"

        }
    }
}

Answer 2

Apart from Winstons excellent suggestions I would further seperate the checks and parsing into seperate methods. Maybe even class. Excuse any typos as I had to do this in notepad but hopefully the basic idea is below.

public string ConvertToProperNameCase(string input)
{
   input = input.Trim();

   if (String.IsNullOrEmpty(input))
   {
     return String.Empty;
   }

   char[] chars = CultureInfo.CurrentCulture.TextInfo.ToTitleCase(input.ToLower()).ToCharArray();

   for (int i = 0; i + 1 < chars.Length; i++)
   {
       if ((chars[i].Equals('\'')) || (chars[i].Equals('-')))
       {
         chars[i + 1] = Char.ToUpper(chars[i + 1]);
       }
   }

   string s = new string(chars);

   const string[] prefixes = { "MAC", " MCC", "DE ", "MC" });
   string s = ParseData(s, prefixes,HasPrefix, ParsePrefix);

   private const string[] suffixes = { " II", " III", " IV");
   return ParseData(s, suffixes ,HasSuffix, ParseSuffix);
}    

private string ParseData(string input, string[] data, Func<string, string, bool> hasData, Func<string, string, string> getData) 
    {

      foreach(string item in data)
      {
        if (hasData(input,  prefix) 
                {
            return getData(input, prefixes);
        }
     }

     return input;
    }

private bool HasPrefix(string input, string prefix)
{
    int length = prefix.length + 1;
    return (s.ToUpper().StartsWith(prefix)) && input.Length > length;
}

private string ParsePrefix(string input, string prefix)
{
    int  length = prefix.length;
    return input.Substring(0, 1).ToUpper() + input.Substring(1, length - 1).ToLower() + s.Substring(length, 1).ToUpper() + s.Substring(length + 1).ToLower();     
}

private string ParseSuffix(string input, string suffix)
{
    int length = suffix.length;
    return s.Substring(0, input.ToUpper().IndexOf(suffix)) + " " + s.Substring(s.ToUpper().IndexOf(suffix), length).ToUpper();
}

private bool HasSuffix(string input, string suffix)
{
return input.ToUpper().Contains(suffix);
}