4
\$\begingroup\$

I must create a function that accepts a string of names all separated by a comma.

Example: "Jack,Matthew,John,Sam"

And return the string of names in the same order but spelled in reverse.

Expected output: "kcaJ,wehttaM,nhoJ,maS"

I completed the function with an inner for loop but I would like to know if maybe running a single for loop and inside using a Linq function or something similar, would have been better?

public string Reverse(string s)
{
    var names = s.Split(",");
    string temp = "";

    for (int i = 0; i < names.Length; i++)
    {
        for (int j = names[i].Length - 1; j >= 0; j--)
        {
            temp += names[i].ElementAt(j);
        }
        temp += i == names.Length - 1 ? "" : ",";
    }

    return temp;
}

Anyway I found this problem very simple even though I tried taking my time on it, I found this solution almost immediately so I went with it. I am thinking maybe if I spent more time on it then I could have implemented this with some sort of Linq function to reverse the spelling of the names and removed the inner loop.

Really any suggestions for improving this small function is appreciated.

\$\endgroup\$
3
2
\$\begingroup\$

Good code, few suggestions.


Instead of having to check each and every time to make sure you can add a "," or not, add it for all the cases, use String.TrimEnd to remove the extra leading ","

            var names = s.Split(",");
            string temp = "";

            for (int i = 0; i < names.Length; i++)
            {
                for (int j = names[i].Length - 1; j >= 0; j--)
                {
                    temp += names[i].ElementAt(j);
                }
                temp += ",";
            }
            return temp.TrimEnd(',');

With that change, we have the ability to foreach loop to further improve the readability of the code.

            string temp = "";
            foreach (var name in s.Split(','))
            {
                for (int j = name.Length - 1; j >= 0; j--)
                {
                    temp += name.ElementAt(j);
                }
                temp += ",";
            }
            return temp.TrimEnd(',');

That's good. Although we can improve this further by using a better (faster and cleaner) method to reverse the individual names, and that is to use String.ToCharArray and Array.Reverse

The idea is to convert the string into a character array, use the inbuilt Reverse method to reverse it, add it to temp.

            string temp = "";
            foreach (var name in s.Split(','))
            {
                char[] NameArray = name.ToCharArray();
                Array.Reverse(NameArray);

                temp = temp + new string(NameArray) + ",";

            }
            return temp.TrimEnd(',');

You can also use StringBuilder which is slightly faster

            StringBuilder reversed = new StringBuilder();

            foreach (var word in str.Split(','))
            {
                char[] singlesentence = word.ToCharArray();
                Array.Reverse(singlesentence);
                reversed.Append(singlesentence);
                reversed.Append(",");
            }
            return reversed.ToString().TrimEnd(',');

Nitpick:

  • Prefer using a better name like reversed compared to temp, it tells more to the reader.

  • Use string.Empty and not ""


Benchmarks

Your original code

        static public string Reverse(string s)
        {
            var names = s.Split(",");
            string temp = "";

            for (int i = 0; i < names.Length; i++)
            {
                for (int j = names[i].Length - 1; j >= 0; j--)
                {
                    temp += names[i].ElementAt(j);
                }
                temp += i == names.Length - 1 ? "" : ",";
            }
            return temp;
        }

New

        static public string Reverse2(string s)
        {
            
            string reversed = string.Empty;
            foreach (var name in s.Split(','))
            {
                char[] NameArray = name.ToCharArray();
                Array.Reverse(NameArray);

                reversed += new string(NameArray) + ",";

            }
            return reversed.TrimEnd(',');
        }

New2

        static public string Reverse3(string str)
        {
            StringBuilder reversed = new StringBuilder();

            foreach (var word in str.Split(','))
            {
                char[] singlesentence = word.ToCharArray();
                Array.Reverse(singlesentence);
                reversed.Append(singlesentence);
                reversed.Append(",");
            }
            return reversed.ToString().TrimEnd(',');
        }

Note that I have static to use the function without an object reference.

I will use the following method to call the two functions multiple times, and benchmark it.

static void Perform(Func<string, string> Function, string names, int iterations, string type)
{

    var watch = System.Diagnostics.Stopwatch.StartNew();
    for (int i = 0;i < iterations;i++)
        Function(names);

    watch.Stop();

    var elapsed = watch.ElapsedMilliseconds;
    Console.WriteLine($"Time taken for {type}: {elapsed} ms");
}
        static void Main()
        {
            string names = "Jack,Dylan,Bobby,Peter";
            const int iterations = 1000000;

            Perform(Reverse,  names, iterations, "Original");
            Perform(Reverse2, names, iterations, "New");
            Perform(Reverse3,  names, iterations, "New2");
       
        }

Here are the results

/*
" Release " Configuration

 Iterations  |   Original  |   New     |   New2
-------------+-------------+-----------+--------
 10 000      |    29   ms  |    5   ms |   5   ms
-------------+-------------+-----------+--------
 100 000     |    226  ms  |    54  ms |   43  ms
-------------+-------------+-----------+--------
 1 000 000   |    1763 ms  |    442 ms |   405 ms


*/

Try it here

\$\endgroup\$
4
  • 3
    \$\begingroup\$ But why use a StringBuilder instead of simply filling a collection and then applying String.Join afterwards (this also does away with the ugly .TrimEnd(','))? Especially considering this: stackoverflow.com/questions/585860/… (Quite frankly, even if the performance of a StringBuilder was better, I'd still opt for the String.Join approach since it is more readable IMHO.) \$\endgroup\$ – BCdotWEB Nov 22 '20 at 12:46
  • \$\begingroup\$ @BCdotWEB Yeah that's true, the thing is String Builder is faster. If compactness and readability weigh-out performance in your project, you're welcome to use it :) \$\endgroup\$ – Aryan Parekh Nov 22 '20 at 12:59
  • \$\begingroup\$ @BCdotWEB You mean using string.Join(",", str.Split(',').Select(x => new String(x.Reverse().ToArray()))); right? \$\endgroup\$ – Aryan Parekh Nov 22 '20 at 13:10
  • \$\begingroup\$ @BCdotWEB I've benchmarked the performance here. To me, the difference is too significant. \$\endgroup\$ – Aryan Parekh Nov 22 '20 at 13:29
2
\$\begingroup\$

You asked about Linq, so I decided to add a way to do it purely with Linq. This performs a bit worse than the StringBuilder examples, but is still 3x faster than the original:

public static string ReverseWithLinq(string str)
{
    return string.Join(
        ',', 
        str
            .Split(',')
            .Select(w => new string(w.Reverse().ToArray())));
}

P.S. I would typically prefer using framework capabilities as much as possible, and only optimize further when necessary.

\$\endgroup\$
1
  • \$\begingroup\$ I just now saw this, thanks for posting this. \$\endgroup\$ – stupidQuestions Dec 11 '20 at 3:09

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.