6
\$\begingroup\$
from string import punctuation, ascii_lowercase

def is_palindrome(string):
    """Return True if string is a palindrome,
    ignoring whitespaces and punctuation.
    """
    new = ""
    for char in string:
        lc = char.lower()
        for x in lc:
            if x in ascii_lowercase:
                new += x

    return new[::-1] == new

# string = "Was it a rat I saw?"
# print(is_palindrome(string))
\$\endgroup\$
  • \$\begingroup\$ What should is_palindrome('èÈ') be? I mean, you are considering only ascii letters, but should we remove from the string any non-ascii letter before the comparison or unicode letters should be taken into account? \$\endgroup\$ – Bakuriu Sep 28 '13 at 9:59
12
\$\begingroup\$
  • You are unnecessarily importing punctuation
  • Avoid reassigning module names such as string in your code
  • A list comprehension would be a simpler way of filtering out the data you don't want

from string import ascii_letters

def is_palindrome(candidate):
    """
    Returns True if candidate is a palindrome, ignoring whitespaces and punctuation.
    """
    stripped = [char.lower() for char in candidate if char in ascii_letters]
    return stripped == stripped[::-1]
\$\endgroup\$
5
\$\begingroup\$

The question was mostly answered, however I think it's not entirely correct to exclude digits and it's more optimal to lower case the whole string instead of lower casing it character by character. And also there's should be check for empty strings. So my version looks like this:

from string import digits, ascii_lowercase

chars = digits + ascii_lowercase

def is_palindrome(s):
    norm = [c for c in s.lower() if c in chars]
    return norm[::-1] == norm != []

However in 2013 you probably want to have Unicode version of the function which can looks like this:

from unicodedata import category

def is_palindrome(u):
    norm = [c for c in u.lower() if category(c)[0] in "LN"]
    return norm[::-1] == norm != []

Some examples for Unicode version of is_palindrome:

>>> is_palindrome(u"...")
False
>>> is_palindrome(u"Was it a rat I saw?")
True
>>> is_palindrome(u"А роза упала на лапу Азора.")
True
\$\endgroup\$
4
\$\begingroup\$

I think we could build up the new string in a nicer way:

from string import ascii_letters

def is_palindrome(s):
    new = ''.join(c.lower() for c in s if c in ascii_letters)
    return new[::-1] == new

If we wanted to avoid lowercasing and stripping non-letters out of the whole string when the first few characters might be enough to determine if it's a palindrome, we could be lazy about doing the transformation. There's also this nice idea of splitting out the filtering and normalizing into separate logic we could pursue. (see Loop like a Native)

from string import ascii_lowercase
from itertools import izip

def just_lowercase_ascii(s):
    for char in s:
        if char.lower() in ascii_lowercase:
            yield char.lower()

def is_palindrome(s):
    for char1, char2 in izip(just_lowercase_ascii(reversed(s)), just_lowercase_ascii(s):
        if char1 != char2:
            return False
    return True

or

def is_palindrome(s):
    all(c1 == c2 for c1, c2 in izip(just_lowercase_ascii(reversed(s)),
                                    just_lowercase_ascii(s)

edit: didn't change parameter name, but string is bad as other answer points out.

\$\endgroup\$
4
\$\begingroup\$

The most readable and fast implementation should be something on the lines of:

from string import ascii_lowercase


def is_palindrome(candidate, letters=set(ascii_lowercase)):
    """
    Return True if candidate is a palindrome, ignoring case and
    ignoring all characters not in letters.

    The letters paramater defaults to all the ASCII letters.

    """
    stripped = [char for char in candidate.lower() if char in letters]
    return stripped == list(reversed(stripped))

If you are considering only short palindromes(and you probably are if you are not dealing with hundred of characters palindrome strings), using stripped[::-1] should be slightly faster than list(reversed(stripped)), although the latter is more readable.

Note that:

  • calling .lower() on each character is pretty bad for performances. You can first transform to lowercase and then simply check for ascii_lowercase. When possible try to avoid useless function calls since they are quite "heavy" when in a tight loop
  • Use sets when you want to check for membership. Their lookup is constant time, while doing character in some_text might need to scan the whole text. This is especially important in loops since using in on strings might produce a quadratic algorithm, while using sets the algorithm could be linear.

Also in your code:

lc = char.lower()
for x in lc:
    if x in ascii_lowercase:
        new += x

This for loop is useless. lc will always be a single character, hence the for will be executed exactly once, as if the code was:

lc = char.lower()
if lc in ascii_lowercase:
    new += lc
\$\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.