Finding palindromes in C#

Posted on

Problem

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);
    }
}

Solution

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.

Leave a Reply

Your email address will not be published. Required fields are marked *