0
\$\begingroup\$

I have recently completed the code challenge for exercise 48 in Learn Python3 the Hard Way. I was looking for any feedback on my solution and any ideas for improvement!

The full task can be found here:

The project involved taking the test code and creating a script that works from it. The test code can be found here (not my upload).

It passes all the tests, but after at other solutions I doubt my solution. Is my solution effective / efficient?

def scan(sentence):
    rebuild = []
    directions = ['north', 'south', 'east']
    verbs = ['go', 'kill', 'eat']
    stops = ['the', 'in', 'of']
    nouns = ['bear', 'princess']

    split_line = sentence.split()

    for word in split_line.copy():
        try:
            if int(word):
                rebuild.append(("number", int(word)))
                split_line.remove(word)
        except ValueError:
            pass

    for word in split_line:
        if word in directions:
            rebuild.append(("direction", word))
        elif word in verbs:
            rebuild.append(("verb", word))
        elif word in stops:
            rebuild.append(("stop", word))
        elif word in nouns:
            rebuild.append(("noun", word))
        else:
            rebuild.append(("error", word))

    return rebuild
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Your question title is completely meaningless. Also describe what your code is supposed to do please. \$\endgroup\$ Apr 11, 2020 at 19:24
  • 2
    \$\begingroup\$ Please can you include the problem statement, currently it's unclear what you are solving. Thank you. \$\endgroup\$
    – Peilonrayz
    Apr 12, 2020 at 0:23
  • 1
    \$\begingroup\$ The problem statement has to be included in the question itself. Links can rot. \$\endgroup\$
    – Mast
    Apr 26, 2020 at 11:01

1 Answer 1

3
\$\begingroup\$

Rule of 3

One of the main things to look for when refactoring code is the same few lines repeated. Some use the rule of three - if the same section is written more than three times the solution is inefficient. Looking at your code, the if-elif-else block seems like a good start to refactoring.

Some changes I would make:

  • Move to a dictionary structure to make expansion easier
  • Use a for loop over key value pairs

Data Structures

This re-structures your data to be in a key-value format using Python's dictionaries. This would allow you have the word "type" as the key and lookup which words are in the entry for the key. The whole if-elif-else block can be cut down to one for loop. This allows you to expand to more word types without needing to write more if else statements. The for loop would not automatically pick up errors so you can use an else statement. This will run whenever the for loop does not reach a break - this means that no words satisfy the values of the keys.

Reviewed code

def scan(sentence):
    rebuild = []
    word_types = {
    "directions": ['north', 'south', 'east'],
    "verbs": ['go', 'kill', 'eat']
    "stops": ['the', 'in', 'of'],
    "nouns": ['bear', 'princess']
    }

    split_line = sentence.split()

    for word in split_line[:]:
        try:
            if int(word):
                rebuild.append(("number", int(word)))
                split_line.remove(word)
        except ValueError:
            pass

    for word in split_line:
        for key, value in word_types.items():
            if word in value:
                rebuild.append(key,word)
                break
        else:
            rebuild.append(("error", word))

I also used [:] to express copying a list, but using .copy() is fine.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you, this was really helpful! I had a feeling a dictionary would be a good alternative, so thank you for explaining how and clearly. \$\endgroup\$ Apr 12, 2020 at 12:20

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