Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/

Source Link

edited May 23, 2017 at 11:33

Community Bot

edited May 23, 2017 at 11:33

Community Bot

Do you really need an iterator here ? What's the point of saving the state of this code ? You clearly want all the possible palindromes from a specified string which to me sounds like I would receive a List<string> containing all the palindromes in there.
Usually converting string to char array is redundant but if you really need to do that you are better off using string.ToCharArray() instead of string.ToArray() detailed explanation why is given here here.
Contains() is O(n) operation so you should avoid it if possible
It might be a good idea to replace that Tuple<string, int> with a class, but I'm not sure what is the point of having it in the first place.

Do you really need an iterator here ? What's the point of saving the state of this code ? You clearly want all the possible palindromes from a specified string which to me sounds like I would receive a List<string> containing all the palindromes in there.
Usually converting string to char array is redundant but if you really need to do that you are better off using string.ToCharArray() instead of string.ToArray() detailed explanation why is given here.
Contains() is O(n) operation so you should avoid it if possible
It might be a good idea to replace that Tuple<string, int> with a class, but I'm not sure what is the point of having it in the first place.

Do you really need an iterator here ? What's the point of saving the state of this code ? You clearly want all the possible palindromes from a specified string which to me sounds like I would receive a List<string> containing all the palindromes in there.
Usually converting string to char array is redundant but if you really need to do that you are better off using string.ToCharArray() instead of string.ToArray() detailed explanation why is given here.
Contains() is O(n) operation so you should avoid it if possible
It might be a good idea to replace that Tuple<string, int> with a class, but I'm not sure what is the point of having it in the first place.

improved formatting

Source Link

edited Dec 27, 2016 at 18:13

Denis

edited Dec 27, 2016 at 18:13

Denis

8.6k
5
33
76

private static bool IsPalindrome(string input)
{
 for (int i = 0; i < input.Length; i++)
 {
 if (input[i] != input[input.Length - 1 - i])
 {
 return false;
 }
 }
 return true;
}

 publicprivate static List<string> GetPalindromes(this string source)
{
 List<string> subsets = new List<string>();
 for (int i = 0; i < source.Length - 1; i++)
 {
 for (int j = i + 1; j <= source.Length; j++)
 {
 if (j - i > 1 && source[j - 1] == source[i])
 {
 string currentSubset = source.Substring(i, j - i);
 if (currentSubset.Length > 1 && IsPalindrome(currentSubset))
 {
 subsets.Add(currentSubset);
 }
 }
 }
 }
 return subsets;
}

public static IEnumerable<string> GetPalindromes(this string source)
{
 for (int i = 0; i < source.Length - 1; i++)
 {
 for (int j = i + 1; j <= source.Length; j++)
 {
 if (j - i > 1 && source[j - 1] == source[i])
 {
 string currentSubset = source.Substring(i, j - i);
 if (currentSubset.Length > 1 && IsPalindrome(currentSubset))
 {
 yield return currentSubset;
 }
 }
 }
 }
}

private static bool IsPalindrome(string input)
{
 for (int i = 0; i < input.Length; i++)
 {
 if (input[i] != input[input.Length - 1 - i])
 {
 return false;
 }
 }
 return true;
}

 public static List<string> GetPalindromes(this string source)
{
 List<string> subsets = new List<string>();
 for (int i = 0; i < source.Length - 1; i++)
 {
 for (int j = i + 1; j <= source.Length; j++)
 {
 string currentSubset = source.Substring(i, j - i);
 if (currentSubset.Length > 1 && IsPalindrome(currentSubset))
 {
 subsets.Add(currentSubset);
 }
 }
 }
 return subsets;
}

public static IEnumerable<string> GetPalindromes(this string source)
{
 for (int i = 0; i < source.Length - 1; i++)
 {
 for (int j = i + 1; j <= source.Length; j++)
 {
 string currentSubset = source.Substring(i, j - i);
 if (currentSubset.Length > 1 && IsPalindrome(currentSubset))
 {
 yield return currentSubset;
 }
 }
 }
 }

private static bool IsPalindrome(string input)
{
 for (int i = 0; i < input.Length; i++)
 {
 if (input[i] != input[input.Length - 1 - i])
 {
 return false;
 }
 }
 return true;
}

private static List<string> GetPalindromes(string source)
{
 List<string> subsets = new List<string>();
 for (int i = 0; i < source.Length - 1; i++)
 {
 for (int j = i + 1; j <= source.Length; j++)
 {
 if (j - i > 1 && source[j - 1] == source[i])
 {
 string currentSubset = source.Substring(i, j - i);
 if (IsPalindrome(currentSubset))
 {
 subsets.Add(currentSubset);
 }
 }
 }
 }
 return subsets;
}

public static IEnumerable<string> GetPalindromes(this string source)
{
 for (int i = 0; i < source.Length - 1; i++)
 {
 for (int j = i + 1; j <= source.Length; j++)
 {
 if (j - i > 1 && source[j - 1] == source[i])
 {
 string currentSubset = source.Substring(i, j - i);
 if (IsPalindrome(currentSubset))
 {
 yield return currentSubset;
 }
 }
 }
 }
}

Source Link

answered Dec 3, 2016 at 21:41

Denis

answered Dec 3, 2016 at 21:41

Denis

8.6k
5
33
76

First and foremost your code is solving a different problem from the one stated in the question Get all the possible palindromes of a string or at least that's what it looks like and as pointed in my comments you need to provide more details and context about the code.

I will try to make a few points concerning your existing code aside from it being overly-complicated :

Do you really need an iterator here ? What's the point of saving the state of this code ? You clearly want all the possible palindromes from a specified string which to me sounds like I would receive a List<string> containing all the palindromes in there.
Usually converting string to char array is redundant but if you really need to do that you are better off using string.ToCharArray() instead of string.ToArray() detailed explanation why is given here.
Contains() is O(n) operation so you should avoid it if possible
It might be a good idea to replace that Tuple<string, int> with a class, but I'm not sure what is the point of having it in the first place.

I will provide an alternative solution to the problem :

Get all the possible palindromes of a string with length more than 1.

First let's start by declaring a method that decides if a string is a palindrome.

 private static bool IsPalindrome(string input)
 {
 for (int i = 0; i < input.Length; i++)
 {
 if (input[i] != input[input.Length - 1 - i])
 {
 return false;
 }
 }
 return true;
 }

Here we are making use of the fact that string is already a char[] and we can simply avoid the redundant call ToCharArray(). Also we are using only a single for loop and we are returning as soon as we find 2 different characters to increase performance.

This can also be converted to a one-liner using LINQ

private static bool IsPalindrome(string input)
{
 return !input.Where((t, i) => t != input[input.Length - 1 - i]).Any();
}

or with C# 6

private static bool IsPalindrome(string input) => !input.Where((t, i) => t != input[input.Length - 1 - i]).Any();

Next we need to actually make use of that method and get all of the possible palindromes of a string :

 public static List<string> GetPalindromes(this string source)
 {
 List<string> subsets = new List<string>();
 for (int i = 0; i < source.Length - 1; i++)
 {
 for (int j = i + 1; j <= source.Length; j++)
 {
 string currentSubset = source.Substring(i, j - i);
 if (currentSubset.Length > 1 && IsPalindrome(currentSubset))
 {
 subsets.Add(currentSubset);
 }
 }
 }
 return subsets;
 }

This chunk of code is what I would like to have as I explained in my first point but if you really need an iterator :

 public static IEnumerable<string> GetPalindromes(this string source)
 {
 for (int i = 0; i < source.Length - 1; i++)
 {
 for (int j = i + 1; j <= source.Length; j++)
 {
 string currentSubset = source.Substring(i, j - i);
 if (currentSubset.Length > 1 && IsPalindrome(currentSubset))
 {
 yield return currentSubset;
 }
 }
 }
 }

lang-cs