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.

I have written two code pieces of to find if a given string is a palindrome:

public static bool FirstMethod(string str)
{
    int i = 0;
    int j = str.Length - 1;

    while (i<j)
    {
        if (str[i] != str[j])
            return false;

        i++;
        j--;
    }

    return true;
}

public static bool SecondMethod(string myString)
{
    string first = myString.Substring(0, myString.Length / 2);
    char[] arr = myString.ToCharArray();
    Array.Reverse(arr);
    string temp = new string(arr);
    string second = temp.Substring(0, temp.Length / 2);
    return first.Equals(second);
}

Please tell me which one is better.

Is there a better method than this?

share|improve this question
3  
@Malachi comparative-review is on-topic. This is not a givemetehzodez question at all — there are two working implementations. –  200_success Jul 29 at 16:23
3  
I'm not sure either of the methods proposed is an actual valid response. Wikipedia defines a palindrome as : A palindrome is a word, phrase, number, or other sequence of symbols or elements that reads the same forward or reversed, with general allowances for adjustments to punctuation and word dividers. Famous examples include "Amor, Roma", "A man, a plan, a canal: Panama" and "No 'x' in 'Nixon'". Neither of the the methods deals with the removal of punctuation or spaces. –  BENBUN Coder Jul 29 at 16:23
5  
Now write a version that takes into account UTF-16 surrogate pairs and Unicode combining characters :-) –  svick Jul 29 at 18:20
1  
How important is performance to you? For readability, I prefer a straight-forward solution: myString == new String(myString.Reverse().ToArray()); –  Chips_100 Jul 30 at 5:49
1  
@Voo I once implemented that using only the methods of the char type, and in was indeed annoying. But later I learned of the StringInfo class which handles splitting a string into text elements. Using it a unicode aware solution is barely more complex than the naive approach. –  CodesInChaos Aug 7 at 16:42

8 Answers 8

up vote 18 down vote accepted

I would prefer the first one for a few reasons.

  1. It's more readable and easily understandable in my opinion.
  2. There's only one string, so less overhead. (Which is negligible unless this is being called a lot.)
  3. There's only one array. Reverse creates a second array.

That being said, an actual review is in order.

  • Methods that return a boolean value should have names of the form IsSomething or HasSomething. A great name would be IsPalindrome.
  • Neither method accepts anything but a string. Numbers can be palindromic too. Wouldn't it be a nice addition if we could pass an integer to it as well? (Without explicitly calling .ToString() before passing it in that is.)
  • I like that you're only checking for i < j instead of i <= j. It saves a useless iteration when there are an odd number of characters.
  • I also like the while loop. It's a lot cleaner than the for loop suggested by another answer.
  • I won't speak much on the second method, because I think @elios provided a nice implementation of it.
share|improve this answer
2  
In my actual code the name of FirstMethod is "IsPalindrome" I named it this way just for the question purpose :)...for the second point yes I failed to check it for palindrome numbers..and for the third point initially I added i<=j then I realized for odd number of characters the middle value would be the same..thanks for pointing out though –  kyle Jul 29 at 17:46
4  
I actually believe that not checking palindromic numbers is a good thing. First of all, it means you have to write less code. Second, I reject the assumption that numbers themselves can be palindromic. For instance, 22 would be a so-called palindromic number. However, 22 is 10110 in binary, which is not palindromic. To check if a number is palindromic, you need to know the number system (base 10, base 2, base 16). –  Sjoerd Job Postmus Jul 29 at 19:52
1  
It would be a nice extension, that I agree on. I was afraid however that it came across as something one must do, instead of as a feature one could add, and if so, only after the need has been discovered. However, for accepting a number, I do think that one should expect a base to be supplied, optionally defaulting to 10 if the language supports it. (By the way, I upvoted. I completely agree with all the other remarks.) –  Sjoerd Job Postmus Jul 29 at 20:11
3  
@ckuhn203: About the "racecar" vs "voiture de course", I do not think this is comparable to needing to know the base. A number in and by itself is not something consisting of separate entities, and only by choosing a representation (binary, decimal, hexadecimal), do we get something that consist of an ordered list of entities. The number one hundred and twenty-one becomes the entities 1, 2, 1 (in precisely that order). That does me to think to add that not only strings can be palindromic, but any list of items (supporting equality). The algorithm chosen would still be my choice for this. –  Sjoerd Job Postmus Jul 29 at 20:17
3  
@WernerCD, racecar in french is a different word to racecar in english. Whereas 0xF and 15 are the same number and mean the same thing. equal(15, 0xF) is true, but equal("racecar", "voiture de course") is false. –  Jonathan. Jul 30 at 18:38

I agree with @ckuhn203 in that the first looks cleaner.

I'm not too familiar with C# and its style, but in C I would definitely prefer using a for loop:

public static bool FirstMethod(string str)
{
    for (int i = 0, j = str.Length - 1; i < j; i++, j--) {
        if (str[i] != str[j]) {
            return false;
        }
    }
    return true;
}
share|improve this answer

Here is the most common solution for this problem:

public static bool IsPalindrome(string s)
{
    char[] array = s.ToCharArray();
    Array.Reverse(array);
    string backwards = new string(array);

    return s == backwards;
}

and if you are using Linq:

public static bool IsPalindrome(string s)
{
    return s == new string(s.Reverse().ToArray());
}

but if you are planning to execute this code thousands of times (which I doubt), your first method would be the fastest.

share|improve this answer

Consider using LINQ:

static bool IsPalindrome(string s)
{
    var characterPairs = s.Zip(s.Reverse(), (l, r) => l == r);
    return characterPairs.Take(s.Length / 2).All(isMatch => isMatch);
}
share|improve this answer
    
there is no Reverse method in System.String –  elios264 Jul 29 at 16:57
2  
@elios264 using System.Linq; –  dss539 Jul 29 at 16:59
    
totally forgot about that, thanks –  elios264 Jul 29 at 17:01
    
dont thing you need to pass a lambda to All if it is going to be identity –  jk. Jul 29 at 21:04
    
@jk. I wish that were true. It would look cleaner. However... Error 1 No overload for method 'All' takes 0 arguments –  dss539 Jul 30 at 15:36

The first one is probably a bit faster, the second could be cleaned up a bit, but is easier to read. Unless you know you have a performance problem, prefer the second one.

For the second option, you should extract two sub-strings from the original input string called "firstHalf" and "secondHalf" corresponding to the first and second half of the input string respectively. Then reverse "secondHalf" and compare.

share|improve this answer

Another way to write this using LINQ, though I'm not sure it's actually clearer:

Enumerable.Range(0, str.Length / 2).All(i => str[i] == str[str.Length - i - 1])
share|improve this answer
    
finally a one line answer! –  elios264 Jul 29 at 18:49
    
@elios264 My answer can also be one line, but I split it for readability. return s.Zip(s.Reverse(), (l,r)=>l==r).Take(s.Length/2).All(b=>b) –  dss539 Jul 29 at 19:15

My computer science teacher would turn over if at least one person didn't respond with an example of recursion. I'm not a C# guy myself, but if you wanted to nerd out I think that would work a little something like this:

public static bool IsPalindrome(string str)
{
    int len = str.length - 1;

    return str[0] == str[len] && (
           len <= 1 || IsPalindrome( str.Substring(1,len) )
           );
}

As long as you are chopping up new strings you could potentially write this so that long strings could be distributed across multiple cores. That's a 1/number_of_cores improvement!

share|improve this answer

I would add that you could make your method an extension method of string. It just emphasises that being a palindrome is a property of a string representation of some sort.

For ease of understanding, the second method stands out. The index based version works, but most people would scratch their heads if they saw it without a hint.

Also it's a premature optimisation to cut the string in half, even though of course it is logical. Much more clear to reverse and compare. If somehow it really is a bottleneck, then cut it in half.

share|improve this answer
1  
I knew right away what that code was doing, why would people scratch their heads? –  Malachi Jul 29 at 16:50
1  
I agree with @Malachi. The first version is actually easier to read to my eyes. However, my original background was in C/C++ so maybe that's why. The information density is higher. #2 looks like some Java Enterprise Architect wrote it. (not a bad thing necessarily) –  dss539 Jul 29 at 16:58
    
The index manipulation version is not obvious at all. I'm assuming someone would be skimming the code, not parsing every little bit. You'd have to catch the fact that i and j are moving opposite to each other and have a think about why it stops when they cross. The second way actually says "reverse" in the code and compares. –  Carlos Jul 29 at 18:42
1  
It's a pretty basic algorithm. If someone I worked with saw it and didn't understand it, I would be concerned about their level of ability. Loops and arrays should be one of the first things you learn as a programmer. –  RubberDuck Jul 29 at 19:02
1  
@ckuhn203 I think Carlos' point wasn't that he didn't understand it, but rather that it took longer to understand than version #2. I can easily see that being true for me if I did not have a background in C/C++. –  dss539 Jul 29 at 19:13

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.