4
\$\begingroup\$

I have written a script where I have a mocked data of couple of words in a list. The point is that I as a user will be able to input a text e.g. hello there world! and with this function I have written it should check if I have anything that matches and if it does it should print out that it matches else not.

import re

# mocked database
from collections import Iterable

get_keyword = [
    "shorts",
    "red banan",
    "hello world",
    "t-shorts",
    "boxers",
]

find_words = re.compile(r'\w+').findall

words = {word.lower() for word in find_words('hello test worldd')}

single_negative: set[str] = set()
multi_negative: list[set[str]] = []

for db_keyword in get_keyword:
    if " " in db_keyword:
        multi_negative.append(set(db_keyword.split()))
    else:
        single_negative.add(db_keyword)


def matches(single: set[str], multi: Iterable[set[str]]) -> bool:
    return (not words.isdisjoint(single)) or any(multi_word <= words for multi_word in multi)


if matches(single_negative, tuple(multi_negative)):
    # Did find any match
    print('Match found!')

else:
    # Did not find matches
    print('Match NOT found!')

There is some scenarios to keep in mind is that e.g.

  1. If we have "hello world" in our list and the word we want to match is "hello there world" -> that should be print out as match as we can find both word hello and world inside "hello there world"
  2. If we have "hello world" in our list and we have the world "hello there" -> that should print out not matched as we need to find both word "hello" and "world" inside "hello there"
  3. If we have "hello" in our list and we have the world "Master, hello there!" -> that should be print out match found as we just want to see if the word hello is inside "Master, hello there!"

Hope anyone have some spare time to see if I could improve anything :D

\$\endgroup\$
4
  • \$\begingroup\$ In real life, how many single-keywords, multi-keywords, and user-provided sentence words in one sentence will there be? \$\endgroup\$
    – Reinderien
    Sep 6, 2022 at 1:33
  • \$\begingroup\$ What is the application? “Bad word” checking or …? \$\endgroup\$
    – AJNeufeld
    Sep 6, 2022 at 4:19
  • 1
    \$\begingroup\$ single and multi-keywords will be around 150-300 - User provides sentence will be up to 20 words, @Reinderien \$\endgroup\$ Sep 6, 2022 at 6:40
  • \$\begingroup\$ @AJNeufeld , that is correct, bad word checking. \$\endgroup\$ Sep 6, 2022 at 6:40

2 Answers 2

5
\$\begingroup\$

Commenting

This is a bad comment.

# mocked database
from collections import Iterable

Iterable is not the database mock. The comment is clearly in the wrong place, which makes it useless.

Bad import

from collections import Iterable

There is no such identifier in collections.

Poor naming

get_keyword = [
    "shorts",
    "red banan",
    "hello world",
    "t-shorts",
    "boxers",
]

The name get_keyword doesn't make sense. This is not a get_xxx function. Nor are the values a keyword. It is a list of words or phrases that you want to detect. Based on the response in the comments, BAD_WORDS might be a suitable name (in CAP_WORDS because it is a global constant).

You should also come up with better names for single_negative and multi_negative.

Bug?

find_words = re.compile(r'\w+').findall

We've just seen "t-shorts" in the list of bad words you're looking for, but since you are using \w+ to split the input sentences into words, if t-shorts appears in the input sentence, it will be split into two "words": "t" and "shorts". Although "t-shorts" will exist in single_negative, it can never be matched. It is only because you are also watching for "shorts" that a sentence containing t-shorts will be flagged. The presence of hyphenated words in your database is misleading, and will likely confuse the user of this code, since they are not matchable.

If you are intending on allowing hyphenated words, you must allow for hyphens in your find_words splitting. While you are at it, you probably should also allow for apostrophes.

find_words = re.compile(r"[-\w']+").findall

Caseless matching

Do not use .lower() for caseless string matching. Python has a special purpose function for this: .casefold(). It is more aggressive, removing all case distinctions in a string

Code organization

Use functions! Right now, your code is unusable. If I wanted to test a different sentence, I have to edit the source code. It is not even obvious where the sentence text should go, for there isn't a sentence = "..." variable. If I want to test two different sentences, I can't; the program as written will only operate on one string.

Reworked code

import re
from typing import Iterable

BAD_WORDS: set[str] = set()
BAD_WORD_COMBOS: list[set[str]] = []

def _load_badwords(bad_words_collection: Iterable[str]) -> None:
    for bad_words in bad_words_collection:
        if ' ' in bad_words:
            BAD_WORD_COMBOS.append(set(bad_words.split()))
        else:
            BAD_WORDS.add(bad_words)

def load_badword_db(*args, **kwargs) -> None:
    bad_words_list: list[str] = []
    raise NotImplementedError() # TODO: replace with your database loading
    _load_badwords(bad_words_list)

def contains_bad_words(sentence: str) -> bool:
    words = set(re.findall(r"[-'\w]+", sentence.casefold()))

    if not words.isdisjoint(BAD_WORDS):
        return True

    if any(bad_word_combo <= words for bad_word_combo in BAD_WORD_COMBOS):
        return True

    return False

if __name__ == '__main__':

    def test_badwords_expected(sentence: str):
        found = contains_bad_words(sentence)

        if found:
            print(f"{sentence}: Match found.")
        else:
            assert f"{sentence}: Match NOT found!"

    def test_no_badwords_expected(sentence: str):
        found = contains_bad_words(sentence)

        if found:
            assert f"{sentence}: Match FOUND!"
        else:
            print(f"{sentence}: Match not found.")

    # Mock database
    _load_badwords([
        "shorts",
        "red banan",
        "hello world",
        "t-shorts",
        "boxers",
        ])

    test_badwords_expected('Hello test world')
    test_badwords_expected('What are t-shorts, anyway?')

    test_no_badwords_expected('Hello test worldd')

See test, or unittest, or other unit testing libraries for information on how to write proper unit tests.

\$\endgroup\$
1
  • \$\begingroup\$ This is very well documented and thank you and ShapeOfMatter for the awesome answers, Well described! \$\endgroup\$ Sep 6, 2022 at 19:56
3
\$\begingroup\$

AJNeufeld's answer covers a lot of good points. In particular, don't be surprised that half his code only exists to check that the "real" code is working correctly; that's totally normal!

A few more points, the first of which is a nice and obvious sibling to "use functions to organize your code":

Avoid Global State

(Actually, it's nice to avoid state altogether, but let's not get picky yet.)

In your code, matches takes two arguments (the word lists) and compares them against a globally defined "sentence" words. Conversely, in AJNeufeld's code, contains_bad_words takes one argument (the sentence) and compares it against two global word-list structures. Combining these approaches is easy.

from collections.abc import Set, Sequence
def contains_bad_words(sentence: str,
                       bads: Set[str],
                       combos: Sequence[Set[str]]
                      ) -> bool:
    ...

(note that collections.abc.Set is closer in meaning to frozenset than set.)

Avoid mutation

The thing that distinguishes a global constant (which are sometimes useful, although they have a habit of becoming liabilities) from global state is that state changes. Especially in AJNeufeld code, notice that there's a designated function _load_badwords for updating the global vars. This is an invitation for someone to call it twice, or to forget to call it at all. Consider instead having a get_badwords that always returns a new data-structure. That data-structure should probably be immutable; you can at least let the type-checker know you're not supposed to mutate them by annotating the function correctly.

A unified "bad words"

Having two structures in two variables that you have to be checking against isn't great. You have options. You could just pass them around as a tuple, or you could build a big fancy class. dataclasses has some nice options. That said, do you really need two different things at all? Representing singleton bad-words as combos of size one may be slightly more computationally expensive, but it'll work fine and be easier to read.

Code example

Since I've gotten this far...

from dataclasses import dataclass
import re
# I'm using python3.8, so the typing is a little clunkier.
from typing import AbstractSet, Iterable

WORD_BREAKER = re.compile(r"[-'\w]+")

@dataclass(frozen=True)
class BadWords:
    combos: AbstractSet[AbstractSet[str]]

    @staticmethod
    def new(bad_words_collection: Iterable[str]) -> "BadWords":
        return BadWords(frozenset(
            frozenset(WORD_BREAKER.findall(phrase))
            for phrase in bad_words_collection
        ))

    def matches(self, sentence: str) -> bool:
        words = frozenset(WORD_BREAKER.findall(sentence.casefold()))
        return any(combo <= words for combo in self.combos)

def get_badword_db(*args, **kwargs) -> BadWords:
    raise NotImplementedError() # TODO: replace with your database loading


if __name__ == '__main__':

    def test_badwords_expected(bad: BadWords, sentence: str):
        assert bad.matches(sentence), f"{sentence}: Match NOT found in {bad}!"

    def test_no_badwords_expected(bad: BadWords, sentence: str):
        assert not bad.matches(sentence), f"{sentence}: Match FOUND in {bad}!"

    # Mock database
    bad = BadWords.new([
        "shorts",
        "red banan",
        "hello world",
        "t-shorts",
        "boxers",
    ])

    test_badwords_expected(bad, 'Hello test world')
    test_badwords_expected(bad, 'What are t-shorts, anyway?')
    test_no_badwords_expected(bad, 'Hello test worldd')
\$\endgroup\$
11
  • \$\begingroup\$ I was torn in what to do about global state. The OP didn’t use any classes; combine with their handle “PythonNewbie”, I decided to eschew introducing classes and stick with global state. This is the obvious next step … or several steps given you’ve further introduced several different kinds of decorators as well, plus frozen sets (to allow sets of sets, sans explanation). Perhaps a bit of a sharp learning curve, but certainly a taste of the expressiveness of more advanced Python code. \$\endgroup\$
    – AJNeufeld
    Sep 6, 2022 at 19:33
  • \$\begingroup\$ Thank you both for the message and as you say @AJNeufeld - This does indeed look alot more complicated compare to yours where you introduce alot of new stuff for me. \$\endgroup\$ Sep 6, 2022 at 19:54
  • 1
    \$\begingroup\$ @AJNeufeld, you're right that there's a lot of stuff in there that looks odd the first time one sees it. (Aside: immutability isn't just to enable inclusion in a set, it's desirable for its own sake.) \$\endgroup\$ Sep 6, 2022 at 20:15
  • 1
    \$\begingroup\$ @PythonNewbie, I noticed that you were using type annotations in your original, which I strongly encourage! But in case you haven't yet found out the hard way: python doesn't innately enforce typing! You need to use an extra tool (usually mypy, which some IDEs have built-in) to check for type-errors. \$\endgroup\$ Sep 6, 2022 at 20:18
  • \$\begingroup\$ Ahh I see I see! However I have a small issue with def get_badword_db(*args, **kwargs) -> BadWords: im not quite sure what you meant there? How would that work? What should be there e.g. instead of the mock data? \$\endgroup\$ Sep 6, 2022 at 20:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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