Palindrome program needs improvement

Question

Here a string is taken as input and the program suppose to check whether the string is palindrome or not. Is there any way the code can be improved? Is it okay to use foreach to break the string into character.

class Program
{
 static void Main(string[] args)
 {
 //input a string
 string orginalStr = Console.ReadLine().ToLower(); ;
 //call the palindrome method
 Console.WriteLine(CheckPalindrome(orginalStr) 
 ? "This is palindrome" 
 : "This is not palindrome");
 Console.ReadKey();
 }
 static bool CheckPalindrome(string orginalStr)
 {
 //call the string reverse method
 var reversedStr = ReverseString(orginalStr);
 if (reversedStr.Equals(orginalStr))
 return true;
 return false;
 }
 static string ReverseString(string orginalStr)
 {
 string reversedStr = "";
 foreach (char ch in orginalStr)
 {
 reversedStr = ch + reversedStr;
 }
 return reversedStr;
 }
}

• paparazzo: I think this is more efficient. codereview.stackexchange.com/questions/188234/…
• AKdeBerg: @paparazzo wow! This approach is really simple yet efficient ...Thanks for sharing :)

Answer 1

All in all this code does, what you want it to do and you separate responsibility by splitting the code in meaningful functions.

I don't like that CheckPalindrome(...) expects the input to be in a certain format (case). In other words: it should do the preparation of the originalStr by it self and check for invalid input:

 static bool CheckPalindrome(string orginalStr)
 {
 if (string.IsNullOrEmpty(originalStr)) return false;
 originalStr = originalStr.ToLower();
 //call the string reverse method
 var reversedStr = ReverseString(orginalStr);
 if (reversedStr.Equals(orginalStr))
 return true;
 return false;
 }

The palindrome check by comparing the original string with its reversed string is rather inefficient. Instead you can compare each char from the start of the string with the same position from the end of the string. You then only have to iterate through the half of it:

static bool IsPalindrome(string word)
{
 if (string.IsNullOrWhiteSpace(word)) return false;
 word = word.ToLower();
 for (int i = 0; i < word.Length / 2; i++)
 {
 if (word[i] != word[word.Length - i - 1])
 return false;
 }
 return true;
}

---

EDIT

If you go multilingual, you may find this approach useful:

static bool IsPalindrome(string word)
{
 if (string.IsNullOrEmpty(word)) return false;
 StringInfo stringInfo = new StringInfo(word.ToLower());
 int length = stringInfo.LengthInTextElements;
 for (int i = 0; i < length / 2; i++)
 {
 if (stringInfo.SubstringByTextElements(i, 1) != stringInfo.SubstringByTextElements(length - i - 1, 1))
 return false;
 }
 return true;
} 

Disclaimer: I have only tested it on Latin strings, so don't hang me if...

• AKdeBerg: Ah, you are right..I really like your method.It is efficient and concise..Thanks :)
• Gerrit0: The empty string should be a palindrome, also, a string purely consisting of spaces should be considered a palindrome. If tabs are mixed in, it might not be a palindrome, but this was already handled by the OP's original implementation.
• user73941: @Gerrit0: You may be right about empty strings/white space strings - it's a matter of definition. Tabs works fine if they correspond in position, but of cause not if you consider '\t' == ' '

Answer 2

• return directly

  Instead of

  if (reversedStr.Equals(orginalStr))
   return true;
  

  you could do return reverseStr.Equals(originalStr)

• Always write {} after an if statement.

  This will make it more clear what is part of the if statement

• Use LINQ to reverse in one go

  String reversedStr = new String(originalStr.Reverse().ToArray())

• gnasher729: reversedStr = char + reversedStr is ABSOLUTELY NOT the same as reversedStr += char.
• oerkelens: Indeed, for string a+b is not the same as b+a. Anyway, it would be better advice to use a 'Stringbuilder' for this instead of creating new strings the whole time.