7
\$\begingroup\$

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;
    }
}
\$\endgroup\$
10
\$\begingroup\$

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

\$\endgroup\$
  • 1
    \$\begingroup\$ Ah, you are right..I really like your method.It is efficient and concise..Thanks :) \$\endgroup\$ – AKdeBerg Sep 2 '18 at 11:39
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ – Gerrit0 Sep 2 '18 at 14:31
  • 1
    \$\begingroup\$ @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' == ' ' \$\endgroup\$ – Henrik Hansen Sep 3 '18 at 7:57
3
\$\begingroup\$
  • 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())

\$\endgroup\$
  • 4
    \$\begingroup\$ reversedStr = char + reversedStr is ABSOLUTELY NOT the same as reversedStr += char. \$\endgroup\$ – gnasher729 Sep 2 '18 at 14:29
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – oerkelens Sep 2 '18 at 14:34

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.