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);
}
Use constants
The cleaner Regex should be a class constant.
Do not put types into the name of the variables.
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();
}
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, if 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.
