python and palindromes

Question

i recently wrote a method to cycle through /usr/share/dict/words and return a list of palindromes using my ispalindrome(x) method here's some of the code...what's wrong with it? it just stalls for 10 minutes and then returns a list of all the words in the file

def reverse(a):
    return a[::-1]

def ispalindrome(a):
    b = reverse(a)
    if b.lower() == a.lower():
        return True
    else:
        return False

wl = open('/usr/share/dict/words', 'r')
wordlist = wl.readlines()
wl.close()
for x in wordlist:
    if not ispalindrome(x):
        wordlist.remove(x)
print wordlist

Answer 1

wordlist = wl.readlines()

When you do this, there is a new line character at the end, so your list is like:

['eye\n','bye\n', 'cyc\n']

the elements of which are obviously not a palindrome.

You need this:

['eye','bye', 'cyc']

So strip the newline character and it should be fine.

To do this in one line:

wordlist = [line.strip() for line in open('/usr/share/dict/words')]

EDIT: Iterating over a list and modifying it is causing problems. Use a list comprehension,as pointed out by Matthew.

Answer 2

Others have already pointed out better solutions. I want to show you why the list is not empty after running your code. Since your ispalindrome() function will never return True because of the "newlines problem" mentioned in the other answers, your code will call wordlist.remove(x) for every single item. So why is the list not empty at the end?

Because you're modifying the list as you're iterating over it. Consider the following:

>>> l = [1,2,3,4,5,6]
>>> for i in l:
...     l.remove(i)
...
>>> l
[2, 4, 6]

When you remove the 1, the rest of the elements travels one step upwards, so now l[0] is 2. The iteration counter has advanced, though, and will look at l[1] in the next iteration and therefore remove 3 and so on.

So your code removes half of the entries. Moral: Never modify a list while you're iterating over it (unless you know exactly what you're doing :)).

Answer 3

I think there are two problems.

Firstly, what is the point in reading all of the words into a list? Why not process each word in turn and print it if it's a palindrome.

Secondly, watch out for whitespace. You have newlines at the end of each of your words!

Since you're not identifying any palindromes (due to the whitespace), you're going to attempt to remove every item from the list. While you're iterating over it!

This solution runs in well under a second and identifies lots of palindromes:

for word in open('/usr/share/dict/words', 'r'):
    word = word.strip()
    if ispalindrome(word):
        print word

Edit:

Perhaps more 'pythonic' is to use generator expressions:

def ispalindrome(a):
    return a[::-1].lower() == a.lower()

words = (word.strip() for word in open('/usr/share/dict/words', 'r'))
palindromes = (word for word in words if ispalindrome(word))
print '\n'.join(palindromes)

Answer 4

It doesn't return all the words. It returns half. This is because you're modifying the list while iterating over it, which is a mistake. A simpler, and more effective solution, is to use a list comprehension. You can modify sukhbir's to do the whole thing:

[word for word in (word.strip() for word in wl.readlines()) if ispalindrome(word)]

You can also break this up:

stripped = (word.strip() for word in wl.readlines())
wordlist = [word for word in stripped if ispalindrome(word)]

Answer 5

You're including the newline at the end of each word in /usr/share/dict/words. That means you never find any palindromes. You'll speed things up if you just log the palindromes as you find them, instead of deleting non-palindromes from the list, too.