5
\$\begingroup\$

I made a WPF Application, called Palindrome Checker, which checks if what you input is a palindrome.

Any and all tips on how to make this code better in all capacities are wanted and appreciated.

public class Check {

    /// <summary>
    /// Method for checking if the word/text is a palindrome.
    /// </summary>
    public static bool IsPalindrome(string text) {

        int min = 0;
        int max = text.Length - 1;

        while (true) {
            if (min > max) {
                return true;
            }

            char a = text[min];
            char b = text[max];

            if (a != b) {
                return false;
            }

            min++;
            max--;
        }
    }
}
public partial class MainWindow : Window {

    public MainWindow() {

        InitializeComponent();

        lblInput.Foreground = Brushes.ForestGreen;
        lblResult.Foreground = Brushes.ForestGreen;
        lblTitel.Foreground = Brushes.ForestGreen;

    }

    /// <summary>
    /// User input and checking the input if the word a palindrome is.
    /// </summary>
    private void InputText_TextChanged(object sender, TextChangedEventArgs e) {

        string text = InputText.Text;

        bool isPalindrome = Check.IsPalindrome(text);

        OutputText.Text = text + (isPalindrome ? " is a palindrome" : " is NOT a palindrome");

        if(InputText.Text == string.Empty)
            OutputText.Clear();
    }
}

\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

Specification

Palindrome

From wikipedia:

A palindrome is a word, number, phrase, or other sequence of characters which reads the same backward as forward, such as madam or racecar or the number 10801. Sentence-length palindromes may be written when allowances are made for adjustments to capital letters, punctuation, and word dividers, such as "A man, a plan, a canal, Panama!", "Was it a car or a cat I saw?" or "No 'x' in Nixon".

We can argue that your specification is stricter/simpler than that of wikipedia.

Unicode compliance

Your algorithm checks characters, not actual glyphs. This is OK for most applications that use ASCII, ANSI or other encodings that represent characters with 16 bits as limit. But for extended unicode, combining characters and other diacritics, your algorithm is flawed.


Design

Guard arguments

You provide a public method, so argument checks are in place.

if (text == null) throw new ArgumentNullException(nameof(text));

While (true) loops

Many consider while (true) bad practice, because it can lead to infinite loops in missing edge cases. Specially when code gets more complex, this is the case. Consider refactoring the loop to use a boolean. However, since this is a trivial and simple algorithm, I understand why you have done it like this.

Unnecessary variables

Since the variables a and b are only used once, there is no need to create variables.

char a = text[min];
char b = text[max];

if (a != b) {
    return false;
}

Shorter:

if (text[min] != text[max]) {
    return false;
}

Lack of variables

Consider creating variables for objects you require on multiple occasions.

lblInput.Foreground = Brushes.ForestGreen;
lblResult.Foreground = Brushes.ForestGreen;
lblTitel.Foreground = Brushes.ForestGreen;
public Brush ForegroundColor { get; } = Brushes.ForestGreen;

lblInput.Foreground = ForegroundColor;
lblResult.Foreground = ForegroundColor;
lblTitel.Foreground = ForegroundColor;

Normalize user input

You could argue that whitespace at the start or end of the input is mostly unintentional, so you should trim it. This depends on your guidelines for best practices concerning user experience.

string text = InputText.Text;

 var text = InputText.Text.Trim();

Formatted strings

Formatted strings are cleaner and better optimized than concatenating strings.

OutputText.Text = text + (isPalindrome ? " is a palindrome" : " is NOT a palindrome");

OutputText.Text = $"text is{(isPalindrome ? " " : " NOT ")}a palindrome";

Unoptimized code flow

This code clears the output after checking for a palindrome. You should put this before the check, and only calculate the palindrome if any non-empty input is provided.

if(InputText.Text == string.Empty)
            OutputText.Clear();

WPF design

For a trivial UI, using the code-behind of a control is fine. Consider using M-V-VM when you decide to extend the design and make it more complex.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Unless I a mistaken, a C# character can hold all Unicodes up to U+FFFF, that is more than just “ASCII or ANSI.” \$\endgroup\$
    – Martin R
    Jul 5, 2019 at 14:21
  • \$\begingroup\$ @MartinR You are right, I should not use the word only here. \$\endgroup\$
    – dfhwze
    Jul 5, 2019 at 14:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.