Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Hoping for input on the clarity of the docstrings, syntax and performance of these two functions both of which should be self-explanatory:

def tell(paragraph, duration=.1):
    """Print paragraph line by line with pause between each line."""
    lines = paragraph.splitlines()
    for line in lines:
        print line.lstrip()
        time.sleep(duration) 

def yinyang(yes, no, prompt = "y/n: ", tries = 3):
    """Return result based on parsed user input with specified number of tries."""
    input = raw_input(prompt)
    word_list = scan(input)
    repeat_prompt = """Does not compute.
                    Please try again to express yes or no."""
    player_decision = parse_sentence(word_list).decision
    try: 
        assert(input and tries > 0)
        input = None
        if player_decision == "affirmative":
            return yes
        elif player_decision == "negative":
            return no
        else:
            tries += 1
            tell(repeat_prompt)
            return yinyang(yes, no, prompt, tries)
    except AssertionError:
        return no                        

The functions, scan() and parse_sentence() are from a module so their docstring (within the module code) should suffice for any needed explanation, right?

Will include them, too:

"""Accept a word string and return a yes, no or error"""

WORD_TYPES = {'affirmative': ['yes', 'y', 'do', 'will', 'sure', 'will', 'ok', 'can', 'would'],
              'negative': ['no', 'n', 'won\'t', 'not', 'nyet', 'can\'t']}

type_words = {word : label for label, words in WORD_TYPES.items() for word in words}

PUNCTUATION = ['.', '?', '!', ',', ':', ';']

def scan(sentence):
    """Strip punctuation and return list containing word and type"""
    sentence_list = sentence.split()
    for idx, word in enumerate(sentence_list):
        for p in PUNCTUATION:
            if p in word:
                word = word.strip(p)
        try:
            lc_word = word.lower()
            word_type = type_words.get(lc_word, None)
            assert word_type is not None
            sentence_list[idx] = (word_type, lc_word)
        except AssertionError:
            lc_word = word.lower()
            sentence_list[idx] = ('error', lc_word)
    return sentence_list

and parse_sentence is here:

class Sentence(object):

    def __init__(self, decision):
        # remember we take ('noun','princess') tuples and convert them
        self.decision = decision[0]

def peek(word_list):
    if word_list:
        word = word_list[0]
        print word[0]
        print word[1]
        return word[0]
    else:
        return None


def match(word_list, expecting):
    if word_list:
        word = word_list.pop(0)

        if word[0] == expecting:
            return word
        else:
            return None
    else:
        return None

def skip(word_list, word_type):
    while peek(word_list) == word_type:
        match(word_list, word_type)

def parse_decision(word_list):
    next_word = peek(word_list)

    if next_word == 'affirmative':
        return match(word_list, 'affirmative')
    elif next_word == 'negative':
        return match(word_list, 'negative')
    else:
        return ('error', 0)

def parse_sentence(word_list):
    decision = parse_decision(word_list)

    return Sentence(decision)
share|improve this question

1 Answer 1

up vote 2 down vote accepted
  • Snippet 1.

The way yinyang is organized is quite unconventional. I presume that

tries += 1

is a typo (tries -= 1 would make much more sense). Even than I don't see a need for a recursion. A simple loop will do.

assert(input) also misses the point. If raw_input() succeeds, an assertion cannot possibly trigger; if not, EOFError will be raised. For the same reason, there's no need to

input = None

Passing yes and no arguments to yinyang need some serious justification. Returning True/False would be as good.

  • Snippet 2.

PUNCTUATION doesn't feel right: string.punctuation is there for a reason.

word.strip(p) is palliative. What if a punctuation mark is in the middle of the word?

A try/except flow is supposed to deal with problems beyond your control. It is not an if substitute. Besides,

    lc.word = word.lower()
    word_type = type_words.get(lc_word, None)

may not possibly raise AssertionError. Thus they do not belong to the try clause.

Also I guess you are trying too hard with affirmatives. y/n is just than enough.

  • Snippet 3.

I don't see a reason for class Sentence to exist. BTW it is not documented at all.

share|improve this answer
    
tries += 1 was a mistake left over from when tries started at 0, before I decided to make it a parameter. I guess I need to hone my understanding of Pythonic EAFP, try/except flow. And need to revisit the string constants. Also figure out simplify the recursive function into a simple loop and clean up the Sentence processing code. The whole set of code still feels like Frankenstein's monster and it was probably premature to post, but your response certainly gives a lot to work on. –  MikeiLL Jul 31 '14 at 15:19
    
Would it be proper to repost the refactored code? –  MikeiLL Jul 31 '14 at 15:20

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.