Finding palindromes in C#

Question

This is my first foray into OOP, so I'd appreciate any kind of advice!

Here is the problem description:

On the first line of the input, you will receive a number specifying how many lines of input to read. After that, the input consists of some number of lines of text that you will read and determine whether or not it is a palindrome.

The only important factor in validating palindromes is whether or not a sequence of letters is the same backwards and forwards. All other types of characters (spaces, punctuation, newlines, etc.) should be ignored, and whether a character is lower-case or upper-case is irrelevant.

Here is my code:

class Program
{
 static void Main(string[] args)
 {
 Console.WriteLine("How many lines will you be entering? ");
 int numberOfLines = Int32.Parse(Console.ReadLine());
 string mergedLine = MergeMultipleLinesIntoOneLine(numberOfLines);
 bool isPalindrome = IsPalindrome(mergedLine);
 Console.WriteLine(isPalindrome ? "Palindrome" : "Not a palindrome");
 }
 public static string MergeMultipleLinesIntoOneLine(int numberOfLines)
 {
 int line = 1;
 var stringResult = new StringBuilder();
 do
 {
 Console.WriteLine("Please enter phrase #{0}: ", line);
 stringResult.Append(Console.ReadLine() + " ");
 line++;
 } while (line <= numberOfLines);
 return stringResult.ToString();
 }
 public static bool IsPalindrome(string someString)
 {
 string regexPattern = @"(?i)[^a-z]";
 string replacement = "";
 string amendedString = Regex.Replace(someString, regexPattern, replacement).ToLower();
 string reverseAmendedString = new string(amendedString.Reverse().ToArray());
 return amendedString.Equals(reverseAmendedString);
 }
}

Answer 1

Use a for loop where fit

public static string MergeMultipleLinesIntoOneLine(int numberOfLines)
{
 var stringResult = new StringBuilder();
 for (var line = 0; line < numberOfLines; line++)
 {
 Console.WriteLine("Please enter phrase #{0}: ", line);
 stringResult.Append(Console.ReadLine() + " ");
 }
 return stringResult.ToString();
}

MergeMultipleLinesIntoOneLine was very weird, when looping a fixed number of times, you should always use a for loop.

Refactor further

isPalindrome does two things:

• It cleans the string.
• It actually checks if it a palindrome.

These are two things, so we need two functions:

public static string clean(string s) 
{
 string regexPattern = @"(?i)[^a-z]";
 string replacement = "";
 return Regex.Replace(s, regexPattern, replacement).ToLower();
}

and:

public static bool IsPalindrome(string someString)
{
 var amendedString = clean(someString);
 string reverseAmendedString = new string(amendedString.Reverse().ToArray());
 return amendedString.Equals(reverseAmendedString);
}

Do not put types into the name of the variables.

I am talking about regexPattern

Use constants

(削除) The cleaner Regex should be a class constant.

private const string cleaner = @"(?i)[^a-z]";

And replacement is unlikely to change, just use ""

clean is now simpler:

public static string clean(string s) 
{
 return Regex.Replace(s, cleaner, "").ToLower();
}

(削除ここまで)

Variables should be declared in the smallest possible scope, so putting the pattern inside the function is best. @Mat's Mug suggests inlining it, and I agree for such small a pattern.

Avoid unnecessary variables

bool isPalindrome = IsPalindrome(mergedLine);

Delete that line, just use:

Console.WriteLine(isPalindrome(mergedLine) ? "Palindrome" : "Not a palindrome");

Also isPalindrome can use less verbose names and be reduced to:

public static bool IsPalindrome(string someString)
{
 var cleaned = clean(someString);
 return cleaned.Equals(amendedString.Reverse().ToArray());
}

Seriously reducing noise.

Single responsibility system

@t3chb0t rightfully noted that isPalindrome still does two things, in fact, it is better to implement it like:

public static bool IsPalindrome(string text)
{
 return text.Equals(text.Reverse().ToArray());
}

And be careful to pass in cleaned strings. Now it really does only one thing.

• t3chb0t: I think IsPalindrome shouldn't clean the string, it should be passed a cleaned one otherwise you have again two things that this method does. Also I would use the helper variable isPalindrome for the sake of easier debugging ;-) perhaps not necessarily in this case but it's a good habit.
• Caridorc: @t3chb0t I think IsPalindrome shouldn't clean the string -> True. Also I would use the helper variable isPalindrome for the sake of easier debugging -> You say when using the debugger, allowing you see all the values of all the variables? I have no experience using that, but code should not be bloated forever for debugging that may only happen once.