3
\$\begingroup\$

I am creating a string extension to validate some input.

My scenario is when we have a string it will format it according to following guideline. If the sample string is “One Two Three Four Five” and the length is <=20 return same. If more than 20 then “One Two Three F Five”. If still length is more than 20 the “One Two T F Five” etc...

Is there any way I can improve this code? I thought it can use recursion?

public static string ToShortName(this string name)
        {
            if (name.Length <= 20) return name;

            var namelist = name.Split(' ');

            var length = name.Length;

            //add spaces
            length = length + namelist.Count() - 1;

            var output = "";
            var indexList = new List<int>();
            for (var i = namelist.Count() - 2; i >=1 ; i--)
            {
                indexList.Add(i);
                if (length - namelist[i].Length <= 20)
                {
                    break;
                }
                length = length - namelist[i].Length;
            }

            foreach (var i in indexList)
            {
                namelist[i] = namelist[i].Substring(0, 1);
            }
            output = string.Join(" ", namelist);

            return output;
        }
\$\endgroup\$
  • \$\begingroup\$ As a side note, I would expect "validation" to return a true/false (or valid/invalid), rather than to return a possibly modified string. You may want to call this something else. \$\endgroup\$ – Michael Urman Dec 11 '13 at 15:31
  • \$\begingroup\$ What should happen when you shorten is as much as you can and it still isn't less than 20 characters? \$\endgroup\$ – svick Dec 11 '13 at 16:11
  • \$\begingroup\$ @svick: This is coming from a field which will be maximum 70 characters \$\endgroup\$ – develop code Dec 12 '13 at 9:42
  • \$\begingroup\$ @developcode It's still possible that can't be shortened, if the last and first words are too long, or if there's too many short words in the middle. \$\endgroup\$ – svick Dec 12 '13 at 10:03
  • \$\begingroup\$ @svick: That's the same thing I raised for Henriks' answer as well. It should have a check before it return the result. May be to return the initials only if the result>20. (I believe no one will have name initial which can occupy 20 characters though:)) \$\endgroup\$ – huMpty duMpty Dec 12 '13 at 10:23
2
\$\begingroup\$

You should remove this

length = length + namelist.Count() - 1;

because length already includes the spaces.

This

length = length - namelist[i].Length;

should be

length -= namelist[i].Length - 1;

because namelist[i].Length-1 is the number of characters that are removed.

Also: why i >=1 - 1 instead of i >= 0?

You can also do it in a single loop like this:

        for (var i = namelist.Count() - 2; i >=1 && length > 20; i--)
        {
            length -= namelist[i].Length - 1;
            namelist[i] = namelist[i].Substring(0, 1);
        }

        return string.Join(" ", namelist);
\$\endgroup\$
  • \$\begingroup\$ Sorry,i >=1 - 1 was mistake. it is i >=1 \$\endgroup\$ – develop code Dec 11 '13 at 11:14
  • \$\begingroup\$ I am running two loops at the moment. Is it a good way of doing? \$\endgroup\$ – develop code Dec 11 '13 at 11:17
  • \$\begingroup\$ @developcode You can use single loop. See my edit. \$\endgroup\$ – Henrik Dec 11 '13 at 11:22
  • \$\begingroup\$ @Henrik: This loop will make everything shorter. like O T T F Five \$\endgroup\$ – huMpty duMpty Dec 11 '13 at 11:23
  • \$\begingroup\$ @huMptyduMpty No. Did you miss the && length > 20? \$\endgroup\$ – Henrik Dec 11 '13 at 11:24
2
\$\begingroup\$
public static string StringCorrection(this string name)

StringCorrection() is not a great name. 1. I don't see any reason to call out the String type in the name. 2. This is not really a correction, it's more like shortening.

Also, I'm not sure that this should be an extension method, because it's something quite specific, it's not generally useful for almost any string.

if (name.Length <= 20) return name;

I think the max length should be a parameter with a default value, so that the method is more general. Even if you don't do that, at least don't repeat the value, use a variable for it.

var output = "";

There doesn't seem to be any reason to declare the variable this early, or to actually have the variable at all. You could just write:

return string.Join(" ", namelist);
\$\endgroup\$
1
\$\begingroup\$

Here is more concise implementation. Also, when length of the string is more than max length, we need to run the loop at least once, so we should use do..while here.

public static string StringCorrection(this string name)
{
    int maxLength = 20;
    if (name.Length <= maxLength) return name;

    string[] nameArray = name.Split(' ');
    int extraLength = name.Length - maxLength;
    int currentIndex = nameArray.Length - 2;

    if (currentIndex <= 0) return name;

    int wordLength = 0;
    do
    {
        wordLength = nameArray[currentIndex].Length;
        nameArray[currentIndex] = nameArray[currentIndex].Substring(0, 1);
        extraLength -= (--wordLength);
        currentIndex--;
    } while (extraLength > 0 && currentIndex > 0);

    return string.Join(" ", nameArray);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I find it hard to suggest using a do-while loop, and your implementation will walk right off the array if the described task isn't possible. But otherwise the advice is spot on. \$\endgroup\$ – Michael Urman Dec 11 '13 at 21:53
  • \$\begingroup\$ Yes, we can add condition to make sure loop doesn't run off the array. I have updated the loop condition to care of that. \$\endgroup\$ – Sajad Deyargaroo Dec 12 '13 at 8:48

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.