5
\$\begingroup\$

I'm trying to improve my dirty data wrangling code into something nice, readable & reliable.

I believe I've done fairly well here, although maybe I should have some docstrings on my functions. Still, I believe it's fairly readable without them.

I'd love some feedback on ways to improve! This script parses raw data from Gray Ward's fantastic public-domain thesaurus into a more readable JSON file.

import sys
import os
import json

# Parses Grady Ward's public-domain thesaurus, available at
# ftp://svr-ftp.eng.cam.ac.uk/pub/comp.speech/dictionaries/moby/

def main():
    database = {}

    if len(sys.argv) < 3:
        print("Usage: make_database <input filename> <ouput filename>")
        sys.exit(1)

    filename = sys.argv[1]

    if not os.path.exists(filename):
        print(f"File {filename} doesn't exist.")
        sys.exit(1)

    # Read file into database
    with open(filename, "r") as file:
        try:
            parse_file(file, database)
            print("Finished reading the file.")
        except Exception as err:
            print(f"Failed to read file {filename} with error {err}")
            sys.exit(1)

    # Write database to output file as JSON
    output_filename = sys.argv[2]
    with open(output_filename, "w+") as file:
        try:
            data = json.dumps(database)
            file.write(data)
            print(f"Finished writing to file {output_filename}")
        except Exception as err:
            print(f"Failed to read write to file {output_filename} with error {err}")
            sys.exit(1)

def parse_file(file, database):
    for line in file:
        (word, thesaurus) = parse_line(line)
        database[word] = thesaurus

def parse_line(line):
    words = line.strip().split(",")
    return (words[0], words[1:])

if __name__ == '__main__':
    main()

I'm still a student -- I'm trying to improve my code readability & maintainability.

It is a commandline utility that can be called like:

make_database <input filename> <output filename>
\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

Welcome to Code Review!

The current code looks clean enough. Since it will be a command line utility, I'd suggest taking up the argparse or click. argparse is a python inbuilt module, so your end package will not have a dependency other than the dictionary files.

The json package provide the method json.dump which can write directly to a file. No need to have extra memory consumption.

Instead of using print statements, I'd go with logging builtin module with possibly multiple handlers, if you want to stream logs to console or file etc.

\$\endgroup\$
1
\$\begingroup\$

A few more short comments:

  1. I find a script easier to read if all necessary functions are defined before being called. This would mean moving parse_file and parse_line above main.

  2. When returning a tuple, or using tuple assignment, you don't need parenthesis:

    def f(x):
        return x, 2*x
    
    a, a2 = f(2)
    
  3. Your parse_file function could be slightly shortened using map:

    def parse_file(file, database):
        for word, thesaurus in map(parse_line, file):
            database[word] = thesaurus
    
  4. In parse_line you could use extended tuple unpacking to give the parts more meaningful names:

    def parse_line(line):
        word, *thesaurus = line.strip().split(",")
        return word, thesaurus
    
  5. There is no need to define database if the number of arguments is not enough. I would move the line to where it is first needed. Or, even better, let parse_file return a dictionary, since you never update an existing dictionary (and even if you did, you could just call dict.update with that returned dictionary):

    def main():
        ...
        try:
            database = parse_file(filename)
            ...
        ...
    
    def parse_file(file):
        return dict(map(parse_line, file))
    
\$\endgroup\$

Your Answer

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

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