Skip to main content
added 69 characters in body
Source Link
J_H
  • 43.4k
  • 3
  • 38
  • 159

The for loop is perhaps more complex than it absolutely has to be, for example there's coupling among a bunch of local variables. Breaking out athe occasoinal helper could improve readability.

The for loop is perhaps more complex than it absolutely has to be. Breaking out a helper could improve readability.

The for loop is perhaps more complex than it absolutely has to be, for example there's coupling among a bunch of local variables. Breaking out the occasoinal helper could improve readability.

Source Link
J_H
  • 43.4k
  • 3
  • 38
  • 159

Ooohhh, look! It comes with unit tests, excellent!

Also, brownie points for the optional type annotations.


This is Just Wrong.

       if all(phrase in input for phrase in phrases):
           return True

Given a distance of zero and even a single unmatched word, we should be returning False. You were looking for

        return all(phrase in input for phrase in phrases)

As it stands the False case falls through to rest of function, which does not appear to be Author's Intent.


I found this a bit opaque / hard-to-maintain:

       ck_words = phrase.split()
       first_word_matches = [
           i for i, x in enumerate(keywords) if x == ck_words[0]
       ]

I'm sure ck_words could be a perfectly nice local identifier. But please offer a # comment that gives me a hint how I should read "ck". Maybe "check"? But that didn't seem to make sense.

It's not obvious to me what's special about the 1st word, e.g. "ummm can i go ..." arguably has "can" as the 1st important word, but .split() makes it the 2nd word.


I would feel better about complex logic in the for first_word_match ... loop if it could be broken out as a helper.

That way it could have its own contract and its own unit tests.


Consider adding a """docstring""" to get_compound_keyword_match.

Consider deleting one or more docstrings from the unit tests. You have lovely method names, so they are largely self-explanatory. If you feel there's still more to say, then yes by all means say it in a docstring.

The tests are all fine as they are. But don't be shy about adding more than one assert to a test. Sometimes it's more convenient that way. We assume all tests are Green. If it turns out it's not obvious what went wrong with a Red test, then you might get serious about running tiny atomic single-assert tests so you know exactly where things went south.


The .main() call at the end is perfectly nice and convenient; feel free to keep it. But please understand that it is pretty usual for folks to run tests with a command like $ pytest or

$ python -m unittest test_compound_keywords.py

Sometimes a Makefile target ($ make test) will mention such a command.

I'm just saying: don't feel like you have to throw that main() call in there.

Also, consider running this with $ pytest --cov, to help you assess whether the tests have exercised both possibilities for each if.


Overall?

Identifiers are well chosen. This code can reasonably be maintained by others.

The for loop is perhaps more complex than it absolutely has to be. Breaking out a helper could improve readability.

LGTM, ship it.