9
\$\begingroup\$

I'm trying to get some feedback on my code for the lexicon exercise (#48) in Learn Python the Hard Way. I have only recently started learning Python and want to make sure I have my fundamentals down before progressing. I have some experience coding (only with kdb+/q in my work environment) but none in Python, so all advice is welcomed.

verb=['go','kill','eat']
direction=['north','south','east','west']
noun=['bear','princess']
stop=['the','in','of']
vocab={'verb':verb,'direction':direction,'noun':noun,'stop':stop}

def scan(sentence):
    wordlist=sentence.split()
    result=[] #initialize an empty list
    for word in wordlist:
        found=False
        for key,value in vocab.items():
            if word.lower() in value:  #convert to lower case so that we can handle inputs with both cases
                result.append((key,word))
                found=True
                break
        if not found:
            try:
                word=int(word)
                result.append(('number',word))
            except ValueError:
                result.append(('error',word))
    return result

Python version 2.6.

\$\endgroup\$
10
\$\begingroup\$
verb=['go','kill','eat']
direction=['north','south','east','west']
noun=['bear','princess']
stop=['the','in','of']

There is not a lot of point in storing these in separate variables only to stick them inside vocab just put the list literals inside vocab

vocab={'verb':verb,'direction':direction,'noun':noun,'stop':stop}

Python convention say to make constant ALL_CAPS. I'd also not abbreviate the names. Code is read more then written, so make it easy to read not to write.

def scan(sentence):
    wordlist=sentence.split()
    result=[] #initialize an empty list

Pointless comment. Assume you reader understand the language. You don't need to explain what [] means.

    for word in wordlist:

I'd use for word in sentence.split():

        found=False

Boolean logic flags are delayed gotos. Avoid them when you can

        for key,value in vocab.items():
            if word.lower() in value:  #convert to lower case so that we can handle inputs with both cases

Your dictionary is backwards. It maps from word types to lists of words. It'd make more sense to have a dictionary mapping from word to the word type.

                result.append((key,word))
                found=True
                break
        if not found:

Rather then this, use an else block on the for loop. It'll be execute if and only if no break is executed.

            try:
                word=int(word)
                result.append(('number',word))

Put this in the else block for the exception. Generally, try to have as little code in try blocks as possible. Also I wouldn't store the result in word again, I'd put in a new local.

            except ValueError:
                result.append(('error',word))

    return result  

My approach:

WORD_TYPES = {
   'verb' : ['go', 'kill', 'eat'],
   'direction' : ['north', 'south', 'east', 'west'],
   'noun' : ['bear', 'princess'],
   'stop' : ['the','in','of']
}
# invert the dictionary
VOCABULARY = {word: word_type for word_type, words in WORD_TYPES.items() for word in words}

def scan(sentence):
    tokens = []
    for word in sentence.split():
        try:
            word_type = VOCABULAR[word]
        except KeyError:
            try:
                 value = int(word)
            except ValueError:
                 tokens.append( ('error',word) )
            else:
                 tokens.append( ('int', value) )
        else:
            tokens.append( (word_type, word) )
     return tokens

Alternately, using some regular expressions:

classifications = []
for word_type, words in WORD_TYPES.items():
    word_expression = '|'.join("(?:%s)" % re.escape(word) for word in words)
    expression = r"\b(?P<%s>%s)\b" % (word_type, word_expression)
    classifications.append(expression)
classifications.append(r"\b(?P<int>\d+)\b")
classifications.append(r"\b(?P<error>\w+)\b")
parser = re.compile('|'.join(classifications))

def scan(sentence):
    return [(match.lastgroup, match.group(0)) for match in parser.finditer(sentence)]

EDIT

If your version of python is to old to use the dict comphrensions you can use:

dict((word, word_type) for word_type, words in WORD_TYPES.items() for word in words)
\$\endgroup\$
  • \$\begingroup\$ thanks for the response! Very much appreciated. Would you mind walking me through the dictionary inversion? I was trying to drop it into my terminal and play with it but I get a SyntaxError:invalid syntax. It seemed to me to be some sort of variant on list comprehension where for each word in words (i.e. for each element in the value-pairs) it assigns the wordtype (i.e. the key in the original dictionary key-value pair) to that element. Not sure why it doesn't work though... \$\endgroup\$ – JPC Aug 2 '12 at 13:00
  • \$\begingroup\$ @JPC, which version of python are you running? The dict comprehension is a newer feature. But you've got the right idea of how it works. I'll add an alternate way \$\endgroup\$ – Winston Ewert Aug 2 '12 at 15:02
  • \$\begingroup\$ seem's I'm running on 2.6.4. Thanks! I appreciate it \$\endgroup\$ – JPC Aug 2 '12 at 17:27
  • \$\begingroup\$ @JPC, yeah I think you need 2.7 for the dict comp. \$\endgroup\$ – Winston Ewert Aug 2 '12 at 17:43

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.