Palindrome program needs improvement

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;




2 Answers
2

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:

CheckPalindrome(...)

originalStr

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

't' == ' '

return directly

return

Instead of

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


you could do return reverseStr.Equals(originalStr)

return reverseStr.Equals(originalStr)

Always write after an if statement.

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

if

Use LINQ to reverse in one go

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

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

Thanks for contributing an answer to Code Review Stack Exchange!

But avoid …

Use MathJax to format equations. MathJax reference.

To learn more, see our tips on writing great answers.

Some of your past answers have not been well-received, and you're in danger of being blocked from answering.

Please pay close attention to the following guidance:

But avoid …

To learn more, see our tips on writing great answers.

Required, but never shown

Required, but never shown

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.