Wordlist formatter code

Question

I am making a "wordlist cleaner" module that loads a words.txt file, and returns a list of cleaned/sorted words.

Full code:

import logging


MIN_WORD_LENGTH = 3
MAX_WORD_LENGTH = 25


class WordlistCleaner:
    def _load_words_from_file(self, words_file: str):
        """Loads a list of words from a file"""
        words = [line.strip() for line in open(words_file, encoding="utf-8").read().lower().splitlines()]
        if len(words) == 0:
            logging.warning(f"No words were found in the {words_file} file.\nExiting script...")
            quit()

        return words

    def _remove_invalid_words(self, words: list[str]):
        """Cleans a list of words by removing words that do not adhere to requirements"""
        valid_words = []

        for word in words:
            if len(word) > MAX_WORD_LENGTH or len(word) < MIN_WORD_LENGTH:
                logging.warning(f"{word} has been excluded - words MIN_word_LENGTH be between 3 and 25 characters.")
                continue

            try:
                word.encode("ascii")
            except UnicodeEncodeError:
                logging.warning(f"{word} has been excluded - words must not contain Unicode characters.")
                continue

            valid_words.append(word)

        return valid_words

    def _remove_duplicates(self, words: list[str]):
        """Removes duplicates from a list"""
        return list(set(words))

    def clean_words(self, words_file: str):
        """Takes a filename / filepath as input and returns the cleaned words"""
        unsorted_words= self._load_words_from_file(words_file)
        valid_words = self._remove_invalid_words(unsorted_words)

        if len(unsorted_words) > len(valid_words):
            logging.warning(
                f"{len(unsorted_words)-len(valid_words)} words have been removed from the list as they were invalid"
            )

        sorted_words = self._remove_duplicates(valid_words)
        if len(valid_words) > len(sorted_words):
            logging.warning(f"{len(valid_words)-len(sorted_words)} duplicate words have been found and removed")

        with open(words_file, "w", encoding="ascii") as f:
            for word in sorted_words:
                f.write(word + "\n")

        logging.info(f"The {words_file} file has been updated.")

        return sorted_words


cleaner = WordlistCleaner()

cleaned_wordlist = cleaner.clean_words("words.txt")

The goal is to just load a wordlist and make sure that every word adheres to requirements. This includes removing duplicates, words with Unicode characters, and words that don't meet character limits.

I was wondering if anyone knew how I could clean up the code a bit, since it feels very messy now. I also have a habit of having this "top down" method execution approach when writing classes. That is, the method at the bottom of the class executes all of the top methods chronologically until it reaches the bottom. Not sure if there's anything wrong with that, but it is definitely a pattern I've noticed in a few of my programs.

Moreover, I'm not sure if it's bad practice to call quit() if an empty file is provided, or if an exception would be more appropriate.

Thank you in advance for any help/suggestions.

Answer 1

There's nothing wrong with the "chronological" aspect of execution.

I like the nice separation of public() vs _private() (helper) methods. Nice MANIFEST_CONSTANTS. Identifiers are spelled as PEP-8 recommends.

---

bad practice to call quit() [?]

Yes, definitely bad practice.

Just raise an informative error and call it a day. If top-level caller drops out, fine, we drop out.

(I assume that quit calls sys.exit(), but TBH it doesn't come up much outside an interactive context, and help(quit) proved rather less than informative.)

---

        words = [line.strip() for line in open(words_file, encoding="utf-8").read().lower().splitlines()]

I know, I know, all the cool kids are doing "one liners".

But rather than leaving an open file descriptor lying around, I would much rather see you use an explicit context handler that will close() when it's no longer needed. If you speak idiomatically it imposes less cognitive burden on the reader, so greater attention can be devoted to the novel pieces that matter.

        with open(words_file, encoding="utf-8") as f:
            words = [line.strip() for line in f.read().lower().splitlines()]

This is just wrong:

            logging.warning(f"No words were found in the {words_file} file.\nExiting script...")
            quit()

Please don't do that. It clearly is not a "warning". Rather, it is a fatal "error", and logging offers a method for that. The appropriate way to signal such trouble is with raise. Could be raise ValueError("No words..."), or with some new error you might choose to define. If caller exits, fine, that's similar to quit(). If caller catches the error and moves on, that's for the caller to decide and not for this low-level code.

This is a lovely docstring:

        """Loads a list of words from a file"""

Concise. Accurate. Consider augmenting the optional type hinting in the signature to reflect that we're returning List[str].

---

This is good:

logging.warning(f"{word} has been excluded ...

but I'm going to quibble with the "3 and 25" part of that string literal, on DRY grounds. You have some lovely constants defined already. So use them!

... between {MIN_WORD_LENGTH} and {MAX_WORD_LENGTH} characters.")

In the words must not contain Unicode characters diagnostic, I get what you mean. But strictly speaking every 7-bit ASCII character is definitely a unicode codepoint. So let's rephrase it as words must be just ASCII characters.

---

In _remove_duplicates we call both set and list, but just set would suffice.

Then later we see

        sorted_words = self._remove_duplicates(valid_words)

I do not understand that identifier at all. These words are in no way sorted.

We could fix it in this way: return sorted(set(words))

---

Overall? Only minor issues, mostly related to reporting errors.

This code achieves its objectives, and is maintainable.

Is this "messy"? No, I'm not seeing that. There are some things we need to compute, and they are addressed in a straightforward way.

No unit tests appeared in this submission.

Answer 2

When possible design classes to be data-oriented rather than effect-oriented. Your current code looks to be thoughtful and carefully done. However, its general approach is one that I have learned (slowly over time, very much the hard way) to avoid. It takes a file path, reads the file content, parses it into words, filters out the unwanted words, emitting logging message along the way, overwrites the original file [?!], and the returns the wanted words. Other that the final return value, (1) most of the data generated along the ways is either lost or buried in logging messages, and (2) the most consequential behaviors of the class are its side effects (logging emitted and file overwritten).

Opened file handles are directly iterable line-by-line. No need to read and split into lines; instead just iterate.

Your class name uses inconsistent camel-case. WordListCleaner seems more apt than WordlistCleaner.

Python 3.7 has an easier way to check for ASCII. Just use word.isascii().

In nearly every context, humans read top to bottom. Whenever feasible, organize your code to support readability, rather than mimicking conventions imposed on programmers by the limits of old programming languages and techniques.

A different approach: files are a separate concern. The primary input to a word list cleaner should be a sequence or iterable of words, not a file path. That makes testing the class much easier, and it maintains a conceptual clarity regarding its purpose. If the main intended use case is dealing with words coming from files, support file reading via a class method (as illustrated below) or a separate utility function. Either approach gives you the desired convenience without corrupting the fundamental data-orientation of WordlistCleaner. Regarding file writing, I would suggest a separate utility function. That said, it would certainly be possible for the class to accept either a file path or an iterable of words as its input. You could store the file path as an attribute, which would support subsequent file overwriting. Under most situations, I would tend to avoid that approach, but it's not unreasonable. In any case, if you add file path as an acceptable input, don't make overwriting the default behavior and don't remove the ability to take an iterable of words as input. The latter behavior is useful for testing and some users of the class might need to read the file on their own and do some pre-filtering before engaging WordlistCleaner. If you bake file reading to the word list cleaner you make any pre-processing needs awkward to handle.

A different approach: don't throw away data, organize it. Here's how I would think about the data flow. Start from the input words and sort them (all_words). Exclude the unneeded duplicate copies (dups). Exclude invalid words (invalids). Everything left is what we want (words). The sum of the latter three should equal all_words. Nothing is lost. All of the information is available to the class user as attributes. And we are under no pressure to report or log anything.

A different approach: when practical, make classes amenable to customization. Let the user control length limits and provide their own word validator. The former is super easy to handle, and the supporting the latter pushed me in the direction of writing more readable and maintainable code by separating valid-vs-invalid partitioning from word validation.

import sys

def main(args):
    # Files are not required.
    WORDS = 'pear apple apple apple banana pear a zz bérry'.split()
    input_words = args or WORDS
    wc = WordListCleaner(input_words)

    # User can check what they want.
    print(wc.words)     # ['apple', 'banana', 'pear']
    print(wc.dups)      # ['apple', 'apple', 'pear']
    print(wc.invalids)  # ['a', 'bérry', 'zz']

    # No information lost.
    assert sorted(wc.words + wc.dups + wc.invalids) == sorted(input_words)

class WordListCleaner:

    def __init__(self, words, min_len = 3, max_len = 25, validator = None):
        # Customizable length limits and validation.
        self.all_words = sorted(words)
        self.min_len = min_len
        self.max_len = max_len
        self.validator = validator or self._is_valid
        self.words, self.dups = self._find_duplicates()
        self.words, self.invalids = self._validate()

    def _find_duplicates(self):
        # Returns the words we want to retain and the extra duplicates.
        words = []
        dups = []
        seen = set()
        for w in self.all_words:
            if w in seen:
                dups.append(w)
            else:
                words.append(w)
                seen.add(w)
        return (words, dups)

    def _validate(self):
        # Returns the words we want to retain and the invalid words.
        groups = ([], [])
        for w in self.words:
            ok = self.validator(w)
            groups[not ok].append(w)
        return groups

    def _is_valid(self, word):
        # The default validator.
        return (
            word.isascii() and
            self.min_len <= len(word) <= self.max_len
        )

    @classmethod
    def from_file(cls, path, *xs, **kws):
        # Returns a WordListCleaner where the words come from a file.
        with open(path) as fh:
            words = [line.strip() for line in fh]
            return cls(words, *xs, **kws)

if __name__ == '__main__':
    main(sys.argv[1:])

Answer 3

Follow the single responsibility principle Your WordListCleaner class is doing too many things. It is in fact not just a word list cleaner but also a word list file loader, parser, cleaner, formatter, and loader. Imagine you want to leverage this same implementation to clean words you scraped from the web. You will now have to open up the WordListCleaner implementation and add a _load_words_from_web function. This brings me to my second point.

Dont use classes where a function can do the job The main objective of your WordListCleaner is to do some clean-up on a list of words. This can be achieved by having a standalone function named get_valid_words. This function can take in a list of words that are "unclean" and return a list of "clean" words.

Here is how I would structure the code:

words = load_words_from_*()
valid_words = get_valid_words()
save_words_to_*(valid_words)

This structure is not only modular, more clear as to where you are getting the words but also testible.

Don't spew useless logs For a small utility like this, it might seem alright to log every single word that was discarded. But imagine the clutter you will have if you integrate this into a larger system. Every time you parse a file say the terminal/logging is full of useless information drowning out the actual useful information. Logs/Comments also tend to drift with time and take the reader's eyes away from the actual information aka the code. Instead of using logs to see if you have correctly implemented something consider writing unit tests.

Use functools Let's get to the actual meat and potatoes of the code aka your _remove_invalid_words function. Firstly, the name is wrong. The function actually returns valid words. It does not remove invalid words from the supplied list. So let's rename that to get_valid_words. This function filters the words based on their size and encoding and then removes duplicates. We can cleanly and clearly state this intent by using functools.filter. Here is my implementation.

def apply_all_filters(filters , iterable):
    return filter(lambda x : all(filter(x) for filter in filters), iterable)

def get_valid_words(words, min_word_length = 3, max_word_length = 25):

    check_size = lambda word : min_word_length < len(word) < max_word_length 
    check_ascii = lambda word : word.isascii()
    remove_duplicates = lambda l : list(set(l))

    return remove_duplicates(apply_all_filters([check_size, check_ascii], words))

Lastly use if __name__ == '__main__'

Here is my cleaned up version.

from functools import filter

def apply_all_filters(filters , iterable):
    return filter(lambda x : all(filter(x) for filter in filters), iterable)

def get_valid_words(words, min_word_length = 3, max_word_length = 25):

    check_size = lambda word : min_word_length < len(word) < max_word_length 
    check_ascii = lambda word : word.isascii()
    remove_duplicates = lambda l : list(set(l))

    return remove_duplicates(apply_all_filters([check_size, check_ascii], words))

def load_words_from_file(file_name):
    with open(file_name, encoding="utf-8") as f:
        words = [line.strip() for line in f.read().lower().splitlines()]

def save_words_to_file(file_name, words):
    with open(file_name, "w", encoding="ascii") as f:
        f.writelines(words)

def main():
    words_file = 'words.txt'
    words = load_words_from_file(words_file)
    valid_words = get_valid_words(words)
    save_words_to_file(words_file, valid_words)


if __name__ == '__main__':
    main()