1
\$\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\$
3
  • 2
    \$\begingroup\$ Your code claims that foo and ba are palindromes. \$\endgroup\$ Commented Feb 24, 2018 at 10:41
  • \$\begingroup\$ The problem is on while clausule: while (i < (word.Length / 2) - 1) Thanks \$\endgroup\$ Commented Feb 24, 2018 at 16:10
  • \$\begingroup\$ I've fixed the code. \$\endgroup\$ Commented Mar 1, 2018 at 1:25

2 Answers 2

Reset to default
7
\$\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\$
5
  • \$\begingroup\$ @DaniloDebiaziVicente You could give it the check if it is the answer. \$\endgroup\$ Commented Feb 24, 2018 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\$ Commented Feb 26, 2018 at 1:04
  • \$\begingroup\$ @DaniloDebiaziVicente Up vote and check are not the same. But no problem. \$\endgroup\$ Commented Feb 26, 2018 at 6:33
  • \$\begingroup\$ @Pararazzi I got it! \$\endgroup\$ Commented Feb 27, 2018 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\$ Commented Sep 3, 2018 at 8:03
3
\$\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\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.