current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I have 2 Truncate string methods.

First:

public static string Truncate(string source, int length)
    {
    if (source.Length > length)
    {
        source = source.Substring(0, length);
    }
    return source;
    }

Second:

public static string Truncate2(string source, int length)
    {
    return source.PadRight(length).Substring(0, length).Trim();
    }

Which of these methods is better, faster?

share|improve this question
3  
Side note: overly simplistic if you might deal with international characters. –  Clockwork-Muse 2 days ago
    
Thanks for accepting my answer. I just noticed Guffa's answer now, he explained it better, with very important additional points, so I suggest to accept his answer instead –  janos yesterday

3 Answers 3

up vote 9 down vote accepted

The question is moot, as the methods give different results. It doesn't matter if a method is faster, better, simpler, nicer, fancier or cooler if it doesn't work properly.

The first method works as expected. It always truncates the string to the desired length, if the string is long enough to truncate. (Note though that this could break a combination of unicode characters used to compose a character, but that's a whole different level of expectations.)

The second method will behave badly if the string that you try to truncate contains whitespace characters in the "wrong" place.

For example, Truncate2("A B", 3) returns "A" instead of "A ".

The method will even crash in some situations where it should just return the input unchanged. To get an OutOfMemoryException you can call Truncate2("", Int32.MaxValue).

share|improve this answer
up vote 8 down vote

The first is better. It uses a straightforward logic. The second uses unintuitive, convoluted logic (pad -> substring -> trim). It should be slower for short strings due to the extra padding step. It should be slower for both due to the extra trimming step.

Also keep in mind that the trimming is an additional behavior that the first one doesn't have, and which may shorten the string below the required length, if it contains trailing spaces. So the end result of the two methods is not the same for some inputs.

The first one can be improved:

public static string Truncate(string source, int length)
{
    if (source.Length > length)
    {
        return source.Substring(0, length);
    }
    return source;
}

It's better not to reassign parameter variables. This one avoids that by returning the substring early.

The braces of the the function are also better placed.

share|improve this answer
up vote 6 down vote

janos's answer is entirely correct and I can add nothing to his reasoning about which method is superior. I was intrigued as to why you would want to try the second one at all. I suspect you want more concise code and to avoid the if control block. A more concise version of your first method can be done without the drawbacks of the second:

public static string Truncate(string source, int length)
{
    return ( source.Length > length ? source.Substring(0, length) : source);
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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