4
\$\begingroup\$

This is a basic spell checking program that I wrote earlier today. It accomplishes the tasks that I had in mind, but was curious about improving my coding style and improving efficiency of my program. Any advice is appreciated. I have read through the PEP-8 style guide and some others as background information. The hard-coded wordlist.txt is a text file containing some 60 odd thousand English words that are spelled correctly.

#---------------------------------------------------------
# The "spellCheck" function determines whether the input
# from the inputFile is a correctly spelled word, and if not
# it will return the word and later be written to a file
# containing misspelled words
#---------------------------------------------------------
def spell_check(word, english):
    if word in english:
        return None
    else:
        return word

#---------------------------------------------------------
# The main function will include all of the code that will
# perform actions that are not contained within our other
# functions, and will generally call on those other functions
# to perform required tasks
#---------------------------------------------------------
def main():
    # Grabbing user input
    inputFile = input('Enter the name of the file to input from: ')
    outputFile = input('Enter the name of the file to output to: ')
    english = {}  # Will contain all available correctly spelled words.
    wrong = []  # Will contain all incorrectly spelled words.
    num = 0  # Used for line counter.

    # Opening, Closing, and adding words to spell check dictionary
    with open('wordlist.txt', 'r') as c:
        for line in c:
            (key) = line.strip()
            english[key] = ''

    # Opening, Closing, Checking words, and adding wrong ones to wrong list
    with open(inputFile, 'r') as i:
        for line in i:
            line = line.strip()
            fun = spell_check(line, english)
            if fun is not None:
                wrong.append(fun)

    # Opening, Closing, and Writing to output file
    with open(outputFile, 'w') as o:
        for i in wrong:
            o.write('%d %s\n' % (num, i))
            num += 1

main()
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review, good job with your first post! \$\endgroup\$ – Mathieu Guindon Nov 16 '16 at 2:47
4
\$\begingroup\$

In python your spell_check function could, and should be written like this:

def spell_check(word, english):
    return word if word in english else None

Dict and sets has there differences, one being that membership testing is faster with sets, they also consume less memory, which will be important for the efficiency of your program.

So a pythonic way of defining "english" would be something inline with:

with open("wordlist", "r") as f:
    english = {word.strip() for word in f}

you could still define the set above for readability.


Now that you have a set that you could use for membership testing, I think it's remove the function spell check, since it adds a line of code and somewhat obscures the program. If it did something more then checked for membership, sure, bur right now, no.

In that case, you could remove the third for loop and create a generator to let the program deal with only one word at the time and further reduce memory usage.

with open("check", "r") as f:
    with open("output", "w") as output:
        wrong = (x.strip() for x in f if x.strip() not in english)
        for key, word in enumerate(wrong):
            output.write("{}: {}\n".format(key, word))

what happens here is that the two files are open at the same time, and since wrong is a generator, it will read one line, in this case word, at the time. If the currently written word is in english it yield it and write it to the file.

Pythons enumerate builtins purpose is for these kinda situations, where you want to iterate over something while still keeping track of the rounds.


You should also make it a habit to protect your main function by,

if __name__ == '__main__':
     main()

to avoid executing self written programs when importing them.

Putting it all together:

def main():

    with open("wordlist", "r") as f:
        english = {word.strip() for word in f}

    with open("check", "r") as f:
        with open("output", "w") as output:
            wrong = (x.strip() for x in f if x.strip() not in english)
            for key, word in enumerate(wrong):
                output.write("{}: {}\n".format(key, word))



if __name__ == '__main__':
    main()
\$\endgroup\$
  • \$\begingroup\$ "membership testing is faster with sets" - I've just tested this and the results are interesting, see stackoverflow.com/questions/40633006/…. I still totally agree that sets are the right thing to use here - but more as a case of "authorial intent matters more than performance". \$\endgroup\$ – ymbirtt Nov 16 '16 at 13:48
  • \$\begingroup\$ Much appreciated response. I'll be sifting through all of the advice later today. Thanks again \$\endgroup\$ – beJeb Nov 16 '16 at 21:29
3
\$\begingroup\$

A few comments:

  1. english could be a set rather than map - this is a better logical representation of your intent. In other words, english = {} can be replaced by english = set()
  2. Not sure why the parentheses are needed here (key) = line.strip()
  3. The role of the line counter is unclear. If your intent is for it to represent the line in the input file where the wrong word was found, then it does not achieve its intent. Irrespective of the input file, it will always have 1, 2, 3, and so forth as the line number in the output.
  4. An easy optimization would be to pickle the english words set into a file, and then unpickle it on startup. The set is static, and more than likely you're spending more time on building it than on the spell check itself.
  5. The contract of the spell_check function could be improved a bit. It's more intuitive for the function to return a Boolean. Also, is_word_in_dictionary is a clearer name. Note also that the documentation comment refers to its pre-PEP-8 name. The function can also be reimplemented as one line: return word in english
  6. It's nitpicking, but I'd rename the english variable to language_dict or something similar. Nothing in your code apart from the variable name assumes that your dictionary is English and not French, for example.

Very readable code overall - thanks for making the review easy.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the pointers. I changed my line counter to work by setting the value the key is associated with to the line number \$\endgroup\$ – beJeb Nov 16 '16 at 21:30
2
\$\begingroup\$

Your code is very legible and well laid out. Very easy to make sense of.

My two cents:

  • You could automatically generate an output file name from the input (i.e "file.errors" from "file.txt")

  • It may be nicer for the user to supply the file name as an argument (i.e you call it as python check my_file)

  • For improved efficiency or a challenge, you could implement your dictionary as a trie. This data structure will result in significantly reduced memory usage and search time. (Even more so than a standard set/dict)

  • For even more efficiency, you could store the dictionary as a pickled object that gets loaded upon execution. This should load much faster than reading a text file line by line

\$\endgroup\$
  • \$\begingroup\$ Thanks for the advice. I'll have to look into the more efficient methods you've mentioned \$\endgroup\$ – beJeb Nov 16 '16 at 21:31
1
\$\begingroup\$

The comments describing your functions should all be docstrings. This way you can do e.g. help(main) in an interactive session to see what the function is about and how to use it.

Have a look at PEP-257 for docstring conventions.

\$\endgroup\$
  • \$\begingroup\$ I'll try to do this in the future. This project was actually for a class that I am in, so I was requited to do some things with the code that I otherwise would not have done. Thanks! \$\endgroup\$ – beJeb Nov 16 '16 at 21:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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