Palindrome program needs improvement

.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;

up vote
7
down vote

favorite

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;

asked Sep 2 at 10:09

AKdeBerg

384

1

I think this is more efficient. codereview.stackexchange.com/questions/188234/â€¦
â€“Â paparazzo
Sep 2 at 16:32

@paparazzo wow! This approach is really simple yet efficient ...Thanks for sharing :)
â€“Â AKdeBerg
Sep 2 at 16:53

add a commentÂ |Â

up vote
7
down vote

favorite

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;

asked Sep 2 at 10:09

AKdeBerg

384

1

I think this is more efficient. codereview.stackexchange.com/questions/188234/â€¦
â€“Â paparazzo
Sep 2 at 16:32

@paparazzo wow! This approach is really simple yet efficient ...Thanks for sharing :)
â€“Â AKdeBerg
Sep 2 at 16:53

add a commentÂ |Â

up vote
7
down vote

favorite

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;

asked Sep 2 at 10:09

AKdeBerg

384

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;

asked Sep 2 at 10:09

AKdeBerg

384

asked Sep 2 at 10:09

AKdeBerg

384

asked Sep 2 at 10:09

AKdeBerg

384

asked Sep 2 at 10:09

AKdeBerg

384

1

I think this is more efficient. codereview.stackexchange.com/questions/188234/â€¦
â€“Â paparazzo
Sep 2 at 16:32

@paparazzo wow! This approach is really simple yet efficient ...Thanks for sharing :)
â€“Â AKdeBerg
Sep 2 at 16:53

add a commentÂ |Â

1

I think this is more efficient. codereview.stackexchange.com/questions/188234/â€¦
â€“Â paparazzo
Sep 2 at 16:32

@paparazzo wow! This approach is really simple yet efficient ...Thanks for sharing :)
â€“Â AKdeBerg
Sep 2 at 16:53

I think this is more efficient. codereview.stackexchange.com/questions/188234/â€¦
â€“Â paparazzo
Sep 2 at 16:32

@paparazzo wow! This approach is really simple yet efficient ...Thanks for sharing :)
â€“Â AKdeBerg
Sep 2 at 16:53

add a commentÂ |Â

2 Answers
2

active

oldest

votes

up vote
9
down vote

accepted

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

edited Sep 3 at 8:05

answered Sep 2 at 11:24

Henrik Hansen

4,1831417

1

Ah, you are right..I really like your method.It is efficient and concise..Thanks :)
â€“Â AKdeBerg
Sep 2 at 11:39

3

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.
â€“Â Gerrit0
Sep 2 at 14:31

1

@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' == ' '
â€“Â Henrik Hansen
Sep 3 at 7:57

add a commentÂ |Â

up vote
3
down vote

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

edited Sep 3 at 7:52

Henrik Hansen

4,1831417

answered Sep 2 at 11:24

Ludisposed

6,11121758

4

reversedStr = char + reversedStr is ABSOLUTELY NOT the same as reversedStr += char.
â€“Â gnasher729
Sep 2 at 14:29

1

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.
â€“Â oerkelens
Sep 2 at 14:34

add a commentÂ |Â

Your Answer

StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);

);

draft saved

draft discarded

StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f202978%2fpalindrome-program-needs-improvement%23new-answer', 'question_page');

);

Post as a guest

Name

2 Answers
2

active

oldest

votes

2 Answers
2

active

oldest

votes

up vote
9
down vote

accepted

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;

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

edited Sep 3 at 8:05

answered Sep 2 at 11:24

Henrik Hansen

4,1831417

1

Ah, you are right..I really like your method.It is efficient and concise..Thanks :)
â€“Â AKdeBerg
Sep 2 at 11:39

3

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.
â€“Â Gerrit0
Sep 2 at 14:31

1

@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' == ' '
â€“Â Henrik Hansen
Sep 3 at 7:57

add a commentÂ |Â

up vote
9
down vote

accepted

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;

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

edited Sep 3 at 8:05

answered Sep 2 at 11:24

Henrik Hansen

4,1831417

1

Ah, you are right..I really like your method.It is efficient and concise..Thanks :)
â€“Â AKdeBerg
Sep 2 at 11:39

3

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.
â€“Â Gerrit0
Sep 2 at 14:31

1

@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' == ' '
â€“Â Henrik Hansen
Sep 3 at 7:57

add a commentÂ |Â

up vote
9
down vote

accepted

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;

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

edited Sep 3 at 8:05

answered Sep 2 at 11:24

Henrik Hansen

4,1831417

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;

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

edited Sep 3 at 8:05

answered Sep 2 at 11:24

Henrik Hansen

4,1831417

edited Sep 3 at 8:05

answered Sep 2 at 11:24

Henrik Hansen

4,1831417

answered Sep 2 at 11:24

Henrik Hansen

4,1831417

answered Sep 2 at 11:24

Henrik Hansen

4,1831417

1

Ah, you are right..I really like your method.It is efficient and concise..Thanks :)
â€“Â AKdeBerg
Sep 2 at 11:39

3

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.
â€“Â Gerrit0
Sep 2 at 14:31

1

@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' == ' '
â€“Â Henrik Hansen
Sep 3 at 7:57

add a commentÂ |Â

1

Ah, you are right..I really like your method.It is efficient and concise..Thanks :)
â€“Â AKdeBerg
Sep 2 at 11:39

3

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.
â€“Â Gerrit0
Sep 2 at 14:31

1

@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' == ' '
â€“Â Henrik Hansen
Sep 3 at 7:57

Ah, you are right..I really like your method.It is efficient and concise..Thanks :)
â€“Â AKdeBerg
Sep 2 at 11:39

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.
â€“Â Gerrit0
Sep 2 at 14:31

@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' == ' '
â€“Â Henrik Hansen
Sep 3 at 7:57

add a commentÂ |Â

up vote
3
down vote

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

edited Sep 3 at 7:52

Henrik Hansen

4,1831417

answered Sep 2 at 11:24

Ludisposed

6,11121758

4

reversedStr = char + reversedStr is ABSOLUTELY NOT the same as reversedStr += char.
â€“Â gnasher729
Sep 2 at 14:29

1

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.
â€“Â oerkelens
Sep 2 at 14:34

add a commentÂ |Â

up vote
3
down vote

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

edited Sep 3 at 7:52

Henrik Hansen

4,1831417

answered Sep 2 at 11:24

Ludisposed

6,11121758

4

reversedStr = char + reversedStr is ABSOLUTELY NOT the same as reversedStr += char.
â€“Â gnasher729
Sep 2 at 14:29

1

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.
â€“Â oerkelens
Sep 2 at 14:34

add a commentÂ |Â

up vote
3
down vote

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

edited Sep 3 at 7:52

Henrik Hansen

4,1831417

answered Sep 2 at 11:24

Ludisposed

6,11121758

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

edited Sep 3 at 7:52

Henrik Hansen

4,1831417

answered Sep 2 at 11:24

Ludisposed

6,11121758

edited Sep 3 at 7:52

Henrik Hansen

4,1831417

edited Sep 3 at 7:52

Henrik Hansen

4,1831417

edited Sep 3 at 7:52

Henrik Hansen

4,1831417

answered Sep 2 at 11:24

Ludisposed

6,11121758

answered Sep 2 at 11:24

Ludisposed

6,11121758

answered Sep 2 at 11:24

Ludisposed

6,11121758

4

reversedStr = char + reversedStr is ABSOLUTELY NOT the same as reversedStr += char.
â€“Â gnasher729
Sep 2 at 14:29

1

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.
â€“Â oerkelens
Sep 2 at 14:34

add a commentÂ |Â

4

reversedStr = char + reversedStr is ABSOLUTELY NOT the same as reversedStr += char.
â€“Â gnasher729
Sep 2 at 14:29

1

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.
â€“Â oerkelens
Sep 2 at 14:34

reversedStr = char + reversedStr is ABSOLUTELY NOT the same as reversedStr += char.
â€“Â gnasher729
Sep 2 at 14:29

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.
â€“Â oerkelens
Sep 2 at 14:34

add a commentÂ |Â

draft saved

draft discarded

draft saved

draft discarded

Post as a guest

Name

Search This Blog

Iyfjky