6
\$\begingroup\$

Can someone improve this for me? It seems ugly.

The method should test whether an incoming number is a palindrome and return a Boolean result.

private static bool IsPalindrome(int value)
{
    var inverseValue = string.Concat(value.ToString().Reverse());
    return inverseValue == value.ToString();
}
\$\endgroup\$
  • 5
    \$\begingroup\$ What's ugly? Looks pretty succinct to me. \$\endgroup\$ – Jesse C. Slicer Sep 6 '13 at 0:03
6
\$\begingroup\$

Not much there to be ugly to be honest. I guess all I might consider would be the reverse of MrSmith42 and adding functionality to avoid the duplication of ToString().

private static bool IsPalindrome(int value)
{
    var valueStr = value.ToString();    
    return valueStr == string.Concat(valueStr.Reverse());
}
\$\endgroup\$
1
\$\begingroup\$

Looks o.k. and clear to me. But you can reduce it to one line, if you want.

private static bool IsPalindrome(int value){
    return string.Concat(value.ToString().Reverse()) == value.ToString();
}
\$\endgroup\$
  • \$\begingroup\$ Clever one-liners are rarely the most readable way to go, imho. \$\endgroup\$ – Mathieu Guindon Sep 11 '13 at 23:45
  • \$\begingroup\$ @retailcoder: I think this one-liner is quite easy to read. But I agree, you should always prefer easy to read variants. \$\endgroup\$ – MrSmith42 Sep 12 '13 at 9:35
1
\$\begingroup\$

Well it totally depends on what you want as a "clean" result.

If the conversion to string for the reverse bothers you, you can make an extension method to hide it :

public static class Extensions
{
    public static int Reverse(this int input)
    {
        return Convert.ToInt32(string.Concat(input.ToString().Reverse()));
    }
}

so the original method is reduced to :

private static bool IsPalindrome(int value)
{
    return value == value.Reverse();
}

If you prefer Fluent expressions, you can use .Equals and make it a one liner :

private static bool IsPalindrome(int value)
{
    return string.Concat(value.ToString().Reverse()).Equals(value.ToString());
}

If the duplicate ToString() calls bothers you, you can put it in a temporary variable :

private static bool IsPalindrome(int value)
{
    string valueString = value.ToString();
    string inverse = string.Concat(valueString.Reverse());

    return inverse == valueString;
}

And of course if you want to leave it like that (and maybe change the var to string) it would be correct too, as the original method is truly not horrible.

\$\endgroup\$
0
\$\begingroup\$

Yes, you can do much better! You don't need to use string. I don't know C#, so feel free to edit my answer.

Here is an idea for a function:

function int reverse(int myNumber)
{
    int lastDigit = myNumber % 10;
    if (lastDigit == myNumber)
    {
        return myNumber;
    }
    else
    {
        int myNumberWithLastDigitDeleted = (myNumber - lastDigit) / 10;
        return lastDigit + 10 * reverse(myNumberWithLastDigitDeleted);
    }
}
\$\endgroup\$
  • \$\begingroup\$ This answer is downvoted: I don't have the same opinion as you on what is elegant ! Converting an int to a string to test whether it is a palindrome is IMO not elegant at all! \$\endgroup\$ – colas Sep 11 '13 at 22:47
  • 1
    \$\begingroup\$ I don't know C# either, but converting an int to a string can be elegant if done well. I wasn't the one who downvoted you, by the way. \$\endgroup\$ – Jamal Sep 11 '13 at 23:38
  • 1
    \$\begingroup\$ Not my downvote either, but how is a cryptic, recursive function with variables named i, j and n anywhere near being more elegant than the OP's solution? \$\endgroup\$ – Mathieu Guindon Sep 12 '13 at 0:07
  • \$\begingroup\$ I agree that it is cryptic. Maybe with other names it more clear ! What do you think of the following: to test whether a number is a multiple of 10, I convert it to a string, get the last character and test if it is equal to the character "0" ? \$\endgroup\$ – colas Sep 12 '13 at 0:22
  • 1
    \$\begingroup\$ +1 for using math on numbers instead of string manipulation. It would need to be benchmarked, but certainly a viable option. \$\endgroup\$ – RubberDuck Sep 7 '14 at 2:17

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.