0
\$\begingroup\$

This code could be better?

This is the algorithm:

  1. Compare the 1st character to the last character
  2. Compare the 2nd character to the second last character and so on
  3. Stop when the middle of the string is reached
  4. Just words
namespace Palindrome
{
    class Class1
    {
        static void Main(string[] args)
        {
            while (true)
            {
                Console.WriteLine("Digit a word:");
                string word = Console.ReadLine();

                if (word == "exit")
                    break;

                VerifyPalindrome(word);
            }
        }

        private static void VerifyPalindrome(string word)
        {
            bool compare = true;
            int i = 0;
            while (i < word.Length- 1)
            {
                if (compare)
                {
                    for (int j = word.Length - 1; j >= 0; j--)
                    {
                        if (word[i] == word[j])
                            compare = true;
                        else
                        {
                            compare = false;
                            break;
                        }
                        i++;
                    }
                }
                else
                    break;
            }

            if (compare)
                Console.WriteLine("This is a Palindrome word");
            else
                Console.WriteLine("This is NOT a Palindrome word");
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Your code claims that foo and ba are palindromes. \$\endgroup\$ – Marvin Feb 24 '18 at 10:41
  • \$\begingroup\$ The problem is on while clausule: while (i < (word.Length / 2) - 1) Thanks \$\endgroup\$ – Pankwood Feb 24 '18 at 16:10
  • \$\begingroup\$ I've fixed the code. \$\endgroup\$ – Pankwood Mar 1 '18 at 1:25
6
\$\begingroup\$

As already stated you don't need to set it to true multiple times.
There is cleaner syntax for this.

static bool IsPalidrone(string word)
{
    for (int i = 0, j = word.Length - 1; i < j; i++, j--)
    {
        if (word[i] != word[j])
        {
            return false;
        }
    }
    return true;
}
\$\endgroup\$
  • \$\begingroup\$ @DaniloDebiaziVicente You could give it the check if it is the answer. \$\endgroup\$ – paparazzo Feb 24 '18 at 18:31
  • \$\begingroup\$ I have less than 15 reputation points here. Unfortunately, the Stack Exchange doesn't allow me to check your answer. I'll do that when I got my 15 points. \$\endgroup\$ – Pankwood Feb 26 '18 at 1:04
  • \$\begingroup\$ @DaniloDebiaziVicente Up vote and check are not the same. But no problem. \$\endgroup\$ – paparazzo Feb 26 '18 at 6:33
  • \$\begingroup\$ @Pararazzi I got it! \$\endgroup\$ – Pankwood Feb 27 '18 at 22:48
  • \$\begingroup\$ This answer doesn't account for empty strings, where it will throw trying to access word[-1]. It will also fail on null strings. Whether an empty string is considered a palindrome is debatable. \$\endgroup\$ – cogumel0 Sep 3 '18 at 8:03
2
\$\begingroup\$

One thing I would like to change is the responsibilities of VerifyPalindrome(string word). Remember, your function should only check whether word is a palindrome, nothing else. It shouldn't print out a value, but instead return a boolean that verifies wheter the word is a palindrome or not. Change the function to:

private static bool VerifyPalindrome(string word)
{
    bool compare = true;

    // ... computing code

    return compare;
}

And in Main:

if(VerifyPalindrome(word))
{
    Console.WriteLine("This is a Palindrome word");
}
else
{
    Console.WriteLine("This is NOT a Palindrome word");
}

The other thing I would change is your palindrome logic. Instead of keeping track of a variable compare for whether a string is a palindrome or not and using breaks, which increases the complexity of your code, you can reduce the function to the following, with multiple returns:

private static bool VerifyPalindrome(string word)
{
    int i = 0;
    while (i < (word.Length / 2) - 1)
    {
        for (int j = word.Length - 1; j >= 0; j--)
        {
            if (word[i] != word[j])
                return false;
            i++;
        }
    }
    return true;
}

No you could further shorten this by combining the two loops into one, but I'll leave that as an exercise to you.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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