8
\$\begingroup\$

I took a challenge on CodeEval. Although the code seems to work for the examples taken from the site, I feel it is not really pretty and must be more complicated than it should be.

Description:

The sentence 'A quick brown fox jumps over the lazy dog' contains every single letter in the alphabet. Such sentences are called pangrams. You are to write a program, which takes a sentence, and returns all the letters it is missing (which prevent it from being a pangram). You should ignore the case of the letters in sentence, and your return should be all lower case letters, in alphabetical order. You should also ignore all non US-ASCII characters.In case the input sentence is already a pangram, print out the string NULL.

import sys
filepath = sys.argv[1]
f = open(filepath)
wholealphabet = ('a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r',
                 's','t','u','v','w','x','y','z')
for line in f:
    sortedletters = list(set(line.lower()))
    i = 0
    while i != len(sortedletters):
        if wholealphabet.count(sortedletters[i]) != 0: 
            i = i + 1
        else:
            sortedletters.pop(i)
    missingletters = ""
    for letter in wholealphabet:
        if sortedletters.count(letter) == 0:
            missingletters +=letter
    if len(missingletters) == 0:
        print("NULL")
    else:
        print(missingletters)
\$\endgroup\$
12
\$\begingroup\$

One of Python's greatest strengths is its built-in capability to use sets directly. I don't feel you've used sets to their fullest extent here. I'd also like to point out the with statement, which you should probably use to handle file handles.

from __future__ import with_statement
import sys
from string import ascii_lowercase
filepath = sys.argv[1]
wholealphabet = frozenset(ascii_lowercase)

# Use with to handle file … handles
with open(filepath) as f:
    for line in f: # assume a line is a sentence, not exactly per spec?
        # sortedletters = list(set(line.lower())) # guaranteed to be *unsorted*
        missingletters = wholealphabet.difference(line.lower())
        if missingletters:
            print ''.join(sorted(missingletters))
        else:
            print 'NULL'

That's really all you need. Unless you want to reconsider the definition of a sentence. :)

\$\endgroup\$
  • \$\begingroup\$ wow it really is much simpler that way, didn't know about the with. It seems to work like the using statement in c#. I will do a research on each of the method used as I do not know them. I indeed forgot about actually doing the sort, I made some attempt with the interpreter at first and then pretty much copy pasted in a .py file. What did you mean about reconsidering the definition of a sentence? Wish I could upvote but I do not have the required rank yet. Thank you. \$\endgroup\$ – Tommy Nov 27 '11 at 4:50
  • \$\begingroup\$ Nevermind about the sentence definition I rereaded the answer and understood :) \$\endgroup\$ – Tommy Nov 27 '11 at 5:06
  • \$\begingroup\$ FYI I moved the from future… to the top of the code, because (as I'm sure you've discovered) the code won't work otherwise. \$\endgroup\$ – kojiro Nov 28 '11 at 23:33
  • 1
    \$\begingroup\$ Indeed I had to change it, I also discover that string has a ascii_lowercase method which I used instead of letters.lower() \$\endgroup\$ – Tommy Nov 29 '11 at 2:33
  • \$\begingroup\$ Great catch. So edited! \$\endgroup\$ – kojiro Nov 29 '11 at 2:59
5
\$\begingroup\$
import sys
filepath = sys.argv[1]
f = open(filepath)

I recommend not using one letter variable names (usually). They make the code hard to read.

wholealphabet = ('a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r',
                 's','t','u','v','w','x','y','z')

I'd have made this a string

wholealphabet = "abcdefghijklmnopqrstuvwxyz"

Shorter and works pretty much the same

for line in f:
    sortedletters = list(set(line.lower()))

Yeah, that's not sorted.

    i = 0
    while i != len(sortedletters):
        if wholealphabet.count(sortedletters[i]) != 0: 

This is the same as sortedletters[i] in wholealphabet, which is clearer.

            i = i + 1
        else:
            sortedletters.pop(i)

Modifying a list while iterating over it is bound to be confusing. Its hard to see what you are doing here. The best way in python is usually to create a new list. Like this:

valid_letters = []
for letter in sortedletters:
    if letter in wholealphabet:
        valid_letters.append(letter)

See how much easier it is to see the result of that? In fact, you can even do it more compactly:

valid_letters = [letter for letter in sortedletters if letter in wholealphabet]


    missingletters = ""

Adding to a string can be expensive, I recommend using a list

    for letter in wholealphabet:
        if sortedletters.count(letter) == 0:
            missingletters +=letter

Again, you can simplify this using a list comphrehension

missingletters = [letter for letter in wholealphabet if letter not in sortedletters]


    if len(missingletters) == 0:
        print("NULL")
    else:
        print(missingletters)

As kojrio points out, if you use sets in python you can implement this very easily. His advice to use a with statement is also good.

\$\endgroup\$
  • \$\begingroup\$ I agree that the string is really easier to see that the list of char. I was wondering if there wasn't already a way to test if a char is a valid letter if I had been in c++ I would have casted to a int and would have compared with the ascii table. Indeed forgot the sort that was pretty bad, didn't have enough test case to spot it. I find list comprehension to be hard to read but maybe it's just because I am not used to see it like that. I'll read about it. Thank you for your help. \$\endgroup\$ – Tommy Nov 27 '11 at 5:01
  • 1
    \$\begingroup\$ @Tommy, list comprehensions can be hard to read until you get used to them. \$\endgroup\$ – Winston Ewert Nov 27 '11 at 15:30
  • \$\begingroup\$ yes, I guess it something you get used to just like lambda expression and then start loving. \$\endgroup\$ – Tommy Nov 27 '11 at 15:34

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.