tags:

views:

836

answers:

4

Is there a better way of doing this...

MyString .Trim().Replace("&", "and").Replace(",", "").Replace("  ", " ")
         .Replace(" ", "-").Replace("'", "").Replace("/", "").ToLower();

I've extended the string class to keep it down to one job but is there a quicker way?

public static class StringExtention
{
    public static string clean(this string s)
    {
        return s.Replace("&", "and").Replace(",", "").Replace("  ", " ")
                .Replace(" ", "-").Replace("'", "").Replace(".", "")
                .Replace("eacute;", "é").ToLower();
    }
}

Ta

+13  A: 

Quicker - no. More effective - yes, if you will use the StringBuilder class. With your implementation each operation generates a copy of a string which under circumstances may impair performance. Strings are immutable objects so each operation just returns a modified copy.

If you expect this method to be actively called on multiple strings of significant length, it might be better to "migrate" its implementation onto the StringBuilder class. With it any modification is performed directly on that instance, so you spare unnecessary copy operations.

public static class StringExtention
{
    public static string clean(this string s)
    {
        StringBuilder sb = new StringBuilder (s);

        sb.Replace("&", "and");
        sb.Replace(",", "");
        sb.Replace("  ", " ");
        sb.Replace(" ", "-");
        sb.Replace("'", "");
        sb.Replace(".", "");
        sb.Replace("eacute;", "é");

        return sb.ToString().ToLower();
    }
}
Developer Art
Thanks! I knew that Strings are immutable, I didn't realise it would do it across every replace statement. :o) Ta
Chris M
+3  A: 

this will be more effecient:

public static class StringExtention
{
    public static string clean(this string s)
    {
        return (new StringBuilder(s)).Replace("&", "and").Replace(",", "")
             .Replace("  ", " ").Replace(" ", "-").Replace("'", "")
             .Replace(".", "").Replace("eacute;", "é").ToString().ToLower();
    }
}
TheVillageIdiot
+1  A: 

Maybe a little more readable?

    public static class StringExtension {

        private static Dictionary<string, string> _replacements = new Dictionary<string, string>();

        static StringExtension() {
            _replacements["&"] = "and";
            _replacements[","] = "";
            _replacements["  "] = " ";
            // etc...
        }

        public static string clean(this string s) {
            foreach (string to_replace in _replacements.Keys) {
                s = s.Replace(to_replace, _replacements[to_replace]);
            }
            return s;
        }
    }

Also add New In Town's suggestion about StringBuilder...

Paolo Tedesco
+2  A: 

I think you should use Regular Expressions, RegExp object is the only fastest way to do what you are doing.

Regex reg = new Regex("(\&)|(\,)|(\W)|(eacute\;)", RegexOptions.IgnoreCase);
p = reg.Replace(p, 
(m)=>{  
   switch(m.ToString())
   {
      case "&":
         return "and";
      case "eacute;":
         return "é"; 
   }
   return "";
});
Akash Kava
The problem with the .NET (and otherwise common) Regex implementation is that you can't compose them. In short, the complex replacement operation that's so readably described by the OP's replacements becomes one big harder-to-read regex - and in particular with these kind of alternation-repetition type regexes, it's easy to run into the exponential corner-cases in .NET's Regex engine. So, in practice, unless you're doing *many* replacements, repetitive replacement using stringbuilder is more readable, more reliable (less likely to introduce bugs), and less likely to run very slowly.
Eamon Nerbonne
well i made a function that takes as input the string and 2 char[] with the old chars and the new ones.so all u have to do is to call this funcion and it will generate the regex internally and does the replacements.while considering that some characeters need to be replaced with \ + character
Karim