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;
    }
}

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...

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())