4
\$\begingroup\$

I have to swap the characters in a string before sending said string to a device to show the information sent through a LCD, the method I developed is as follows:

private string StringSwap(string stringToSwap)
{
    if ((stringToSwap.Length % 2).Equals(1))
    {
        stringToSwap += " ";
    }

    char[] array = stringToSwap.ToCharArray();

    for (int i = 0; i < array.Length; i += 2)
    {
        char temp = array[i];
        array[i] = array[i + 1];
        array[i + 1] = temp;
    }
    return new string(array);
}

I want to know if there are any implementations that do this already (I feel like I am reinventing the wheel here) or, in case there is not, know how to optimize and make this code more resilient, while also making it more readable.

Also, should I make this an extension for the string class instead of a method?

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

Here it as an extension. It helps to properly name all the steps:

static class StringSwap
{
    public static string Swap(this string text) =>
        new string(text
            ?.PadToEvenLength()
            ?.SwapOddEvenChars()
            ?.ToArray());

    static string PadToEvenLength(this string text) =>
        text.Length.IsOdd() ? text + ' ' : text;

    static IEnumerable<char> SwapOddEvenChars(this string text) =>
        text.Select((c, i) => i.IsOdd() ? text[i - 1] : text[i + 1]);

    static bool IsOdd(this int number) => 
        number % 2 == 1;        
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I like a lot of things with this code - it's a good solution but there are a couple of problems IMO: ((string)null).Swap() results in a NRE. Implicit access modifiers and poorly name parameters (arguably a non-issue). \$\endgroup\$ – RobH Jan 29 '16 at 8:58
  • \$\begingroup\$ @RobH can you further explain the problem you see? \$\endgroup\$ – Oscar Guillamon Jan 29 '16 at 9:05
  • \$\begingroup\$ @RobH fixed (except implicit access modifiers - it is a definitely way to go for me. The best possible code is the code that does not exist :) \$\endgroup\$ – Dmitry Nogin Jan 29 '16 at 9:08
  • \$\begingroup\$ @RobH Agreed. It looks to me like somebody desparately wanted to use C#'s new features for the sake of using them at the expense of proper error identification. For those reading this that didn't guess NRE = NullReferenceException. Although personally I think it should be a NullArgumentException, but extension methods are a bit of an odd case. \$\endgroup\$ – Pharap Jan 29 '16 at 13:22
  • \$\begingroup\$ @DmitryNogin Personally I don't like your SwapOddEvenChars method's implementation because the variable c is unused. If you want to do this in a linq style you would probably be better off doing Enumerable.Range(0, text.Length).Select(i => i.IsOdd ? text[i - 1] : text[i + 1]); since it better describes the algorithm being used. \$\endgroup\$ – Pharap Jan 29 '16 at 14:27
3
\$\begingroup\$

I haven't run the code, but are you sure it does as you expect it to for odd length strings? By padding the last character with a space, you get some... interesting results.

For example, inputting "foo" will return "of o". That doesn't seem quite right to me. I would expect it to return "ofo". I would do something like this.

private string StringSwap(string stringToSwap)
{
    char[] array = stringToSwap.ToCharArray();

    // even size strings iterate the whole array
    // odd size strings stop one short 
    int offset = (stringToSwap.Length % 2)
    for (int i = 0; i < array.Length - offset; i += 2)
    {
        char temp = array[i];
        array[i] = array[i + 1];
        array[i + 1] = temp;
    }
    return new String(array);
}

Essentially, when the mod of 2 equals zero, the code behaves exactly like yours does now, but when the mod of 2 equals one, the loop exits one iteration early, avoiding the IndexOutOfRangeException that prompted you to hack in that white space.

Also note that I changed

    return new string(array);

To

     return new String(array);

It's terribly picky and pedantic, but I find it poor form to use the string alias when you're using it as a class. Seeing a ctor called on a camelCased identifier just looks weird to me. Same goes for things like string.Empty. But, like I said, that's just me being nit picky and pedantic. It's not actually a problem.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ That would be the right way to go to and I will take that correction you said about odd strings, but the device will always expect an even length string, that is the reason for padding the string to an even length. About being pedantic and nit picky, don't be afraid to be, at least on me, I always want to improve how I write my code and how it looks to other programmers. \$\endgroup\$ – Oscar Guillamon Jan 29 '16 at 10:07
  • 4
    \$\begingroup\$ Ahh. Gotchya. That's kind of an odd contract. [ba dum tiss] \$\endgroup\$ – RubberDuck Jan 29 '16 at 10:26
  • \$\begingroup\$ @RubberDuck To balance the argument, I disagree with your changing string to String. It's a matter of preference so it doesn't matter either way, but as what the OP has used was not technically incorrect or widely accepted as bad practice it shouldn't have been altered. \$\endgroup\$ – Pharap Jan 29 '16 at 13:41
  • \$\begingroup\$ Fair @Pharap. I did call it out as a nitpick, but I do consider it bad practice personally, as it hinders readability in my opinion. \$\endgroup\$ – RubberDuck Jan 29 '16 at 13:52
  • \$\begingroup\$ Also your function fails if passed null, but presumably error checking was 'left as an exercise for the reader'. \$\endgroup\$ – Pharap Jan 29 '16 at 14:18
2
\$\begingroup\$

Finally, an excuse to use the Zip extension method:

private static string StringSwap2(string stringToSwap)
{
    if ((stringToSwap.Length % 2).Equals(1))
    {
        stringToSwap += " ";
    }

    var evens = stringToSwap.Where((w, i) => i % 2 == 0);
    var odds = stringToSwap.Where((w, i) => i % 2 == 1);

    var zipped = evens.Zip(odds, (left, right) => string.Concat(right, left));

    return string.Concat(zipped);
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Evidently I need to brush up on my functional techniques, totally missed that one. \$\endgroup\$ – Pharap Jan 29 '16 at 22:51
  • \$\begingroup\$ Very cool! But this will iterate the list twice, instead of once, right? \$\endgroup\$ – RubberDuck Jan 29 '16 at 22:54
  • \$\begingroup\$ @RubberDuck Sadly yes. Functional programming might be elegant but there is usually a processing cost. Shame there's no 'unzip' function that separates the list into two first :P \$\endgroup\$ – Pharap Jan 29 '16 at 23:07
  • \$\begingroup\$ @RubberDuck, true, but I can still claim that the algorithm is O(N) :) \$\endgroup\$ – user2023861 Feb 1 '16 at 14:09
  • \$\begingroup\$ True, but there are different degrees of O(N). =) \$\endgroup\$ – RubberDuck Feb 1 '16 at 16:00
1
\$\begingroup\$

As a slightly different approach:

public class StringHelper
{
    private static void SwapChars(ref char a, ref char b)
    {
        char c = a;
        a = b;
        b = c;
    }

    public static string StringSwap(string stringToSwap)
    {
        if (stringToSwap == null)
        {
            throw new ArgumentNullException("stringToSwap");
        }

        char[] array = stringToSwap.ToCharArray();

        for (int i = 0; i < array.Length - 1; i += 2)
        {
            StringHelper.SwapChars(ref array[i], ref array[i + 1]);
        }

        return new string(array);
    }
}

The other answers are also correct, but I wanted to present a more traditional alternative.

Also I wanted to intoduce handling null as an exception. Trying to swap a null should generally be treated as an error condition. You may decide that for the sake of simplicity you want to convert a null argument to an empty string or make it return null, that's up to you, but whatever you do you should explicitly handle it as an edge-case.

First note that I have changed i < array.Length to i < array.Length - 1, which has the side effect of making the loop stop short of the final char in a string with an odd length. This behaviour may not be obvious at first so you may prefer to comment or use a variation of one of the other answers to make the behaviour more explicit, but this solution is somewhat more concise than explicit handling of the case where the length is odd.

I chose to make charSwap its own function as a means of separating concerns and making the charSwap function available to future helper functions that may need the functionality, particularly if you are planning to create other string swapping functions with variations on the behaviour of this one.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ All methods should be PascalCase in C#. charSwap -> CharSwap. It would also be better named as SwapCharacters. \$\endgroup\$ – RobH Jan 29 '16 at 16:18
  • \$\begingroup\$ @RobH I made a compromise edit. I'm sure not all conventions forbid private methods being camel-cased, but I usually go by what MSDN says and on this one I can't find anything specific but the general conventions imply all methods should be pascal-cased, so I'll run with that. It's been a while since I've done C# properly, I've been primarily using C++ for the last few weeks/months. \$\endgroup\$ – Pharap Jan 29 '16 at 16:28
  • \$\begingroup\$ The "official" capitalization guidelines don't differentiate method casing based on scope. Although I think any consistent style is fine, most people around CR use Microsoft's style guide. \$\endgroup\$ – RubberDuck Jan 29 '16 at 22:52
  • \$\begingroup\$ @RubberDuck Those are the 'general conventions' I was referring to. \$\endgroup\$ – Pharap Jan 29 '16 at 23:14
  • \$\begingroup\$ Yeah. It's a bit ambiguous @Pharap, but PascalCase regardless of scope is "the standard" for methods. \$\endgroup\$ – RubberDuck Jan 29 '16 at 23:16

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.