1
\$\begingroup\$

I have a few thousand json files containing order histories sitting in a directory from one of our systems. Since customers can opt in to receive marketing I'm opening each json file and then processing the data to scrape our active subscribers. At a later date I plan to expand this to bring this data into our CRM.

As I'm not a Python programmer by trade so I'm wondering what I could do better to make this more functional and (for lack of a better term) 'pythonic'?

import sys, os, json
import pandas as pd

print("Scraping E-mail Subscribers")
print("Using Python version %s.%s.%s" % sys.version_info[:3])

json_path = 'D:/dev/order_archive/'
json_files = [pos_json for pos_json in os.listdir(json_path) if pos_json.endswith('.json')]

records = 0
opted_records = 0
unopted_records = 0

for index, js in enumerate(json_files):
    with open(os.path.join(json_path, js)) as json_file:
        records += 1
        json_text = json.load(json_file)
        first_name = json_text['details']['firstName']
        last_name = json_text['details']['surname']
        newsletter_opt_in = json_text['details']['questions']['newsletterOptIn']
        email = json_text['details']['email']

        if(newsletter_opt_in == 'true'):
            print('Name: %s\nE-mail: %s\n\n' % (first_name + ' ' + last_name, email))
            opted_records += 1
        elif(newsletter_opt_in != 'false'):
            unopted_records += 1

print('Processed %d records. %d opted to receive e-mail marketing. %d didn''t specify an option.' % (records, opted_records, unopted_records))
\$\endgroup\$
1

2 Answers 2

2
\$\begingroup\$

records seems to just count each enumeration of json_files. Just use len(json_files) instead as that returns the length of the json_files list, which is basically what records is.

While we're at it, index is never used, so why enumerate? Just use for js in files.

json_text is a misleading name, because that's actually a dictionary of json data! Name it data or json_data for clarity.

You call on multiple keys any one of which could raise a KeyError if the key doesn't exist or is spelled wrong. You might be very confident in the structure being consistent, but I think it's better to be safe, after all who knows what'd mess with your data or break the structure. There's two ways to ensure that you wont get errors, and I think you'd do well to use both of them.

The first way is to use try except KeyError. This is Python's error catching and it works so well that it's considered more Pythonic than testing if the key is there. However it would stop you from reading other keys if you caught that 'firstName' didn't exist, so I suggest just using this to access json_data['details']. This would also give you a shortcut, rather than needing to type that out fully for each key. Here's how it'd look:

    json_text = json.load(json_file)
    try:
        details = json_text['details']
    except KeyError:
        records -= 1
        print("Bad file {}".format(json_file))
        continue

If a KeyError arises, I'm doing a few things though what you want to do may vary. First, I subtracted one from the records count since we don't have a valid record. You might want to instead track an invalid records number, or not touch that number at all. I also print a message warning the user that one of the files had a problem in it, so they can inspect why it didn't work. You could adjust that message, adding detail about what key was missing would be good but I definitely recommend including the actual name so that they can find the specific file. Lastly I call continue, which will basically go onto the next iteration of your loop. Meaning it immediately skips to the next file, allowing your script to continue.

Now the second method you can use is called get. It's a dictionary method that allows you to specify a default value if the key doesn't exist. I think it would be better here because even if a key is missing you still want to be able to build your string at the end to print. ie. Even if 'surname' isn't included, you could just set it as a blank string and print the rest of the information. So here's how it'd look:

    first_name = details.get('firstName', '')
    last_name = details.get('surname', '')

The first parameter to get is the key name, and the second is your default value. Empty strings seem appropriate, unless you want to highlight that there's missing data by putting in values like "SURNAME" or "VOID". When you need to get a nested value like ['questions']['newsletterOptIn'] you could call get, pass an empty dictionary as default and then call get on that. Basically just chaining get calls will do what you need:

    newsletter_opt_in = details.get('questions', {}).get('newsletterOptIn', '')
    email = details.get('email', '')

Now, when I see your if command I realise that we might not even need most of these keys! So you should reorder the commands so that you just first get newsletter_opt_in and then test that to determine if you even need the other keys:

    newsletter_opt_in = details.get('questions', {}).get('newsletterOptIn', '')
    if(newsletter_opt_in == 'true'):
        first_name = details.get('firstName', '')
        last_name = details.get('surname', '')
        email = details.get('email', '')
        print('Name: {}\nE-mail: {}\n\n'.format(first_name + ' ' + last_name, email))
        opted_records += 1
    elif(newsletter_opt_in != 'false'):
        unopted_records += 1

Also you don't need brackets around the expressions for if statements. if newsletter_opt_in == 'true' is perfectly fine. Also it would be good to add a comment clarifying newsletter_opt_in != 'false'. It took me a minute to realise that it was !=, meaning that you're testing for any value that doesn't match the two boolean possibilities. It's a little confusing but a comment would clear it up easily. Though I recommend switching to actual boolean values in the data if possible. If your source can store newsletter_opt_in as a boolean then Python can read it as such.

Style notes

Instead of putting all your imports on one line you should space them out. Python likes to give each import it's own line and to group imports together. Also since you only need sys for version_info you could use from sys import version_info. That will only import the version_info tuple and makes it clearer why you need that import.

import os
import json

import pandas as pd

from sys import version_info

Using % to format strings is the old way, Python has a better way known as the str.format method. Since you're passing three values out of a tuple, you will need the unpacking operator to make it work. The unpacking operator basically takes values out of a collection (ie. a list or a tuple) and passes them as arguments. So instead of passing a tuple to format we can pass the string elements it contains. To use it, just add the * character before a collection variable.

print("Scraping E-mail Subscribers")
print("Using Python version {}.{}.{}".format(*version_info[:3])

You're using snake_case to name things, which is great! That's exactly what the Python style guide recommends. However since json_path seems to be a constant, you should use UPPER_SNAKE_CASE for it, ie. JSON_PATH.

Speaking of names, pos_json is not great. I assume it's meant to be 'possible json' but that doesn't read well. It's better to name it after what it is, a file. You can't use file as that will shadow a builtin function, but you could use f or filename. Your endswith('.json') makes it pretty clear that it's about detecting if it's a json file so you can use a more direct name that describes what these filenames actually are. Also in your loop, I don't like js for the same reason, and I'd likewise call that filename.

You can also assign multiple values at once in Python, so all your records can be set as 0 simultaneously.

records = opted_records = unopted_records = 0
\$\endgroup\$
1
\$\begingroup\$
json_path = 'D:/dev/order_archive/'
json_files = [pos_json for pos_json in os.listdir(json_path) if pos_json.endswith('.json')]

That code isn't very optimal.

import glob
json_files = glob.iglob('D:/dev/order_archive/*.json')

returns json_files as an iterator. you could also use glob.glob() to get a full list. Your subsequent code can use either an iterator or full list. The comments of the previous answer are also 'pythonic'.

\$\endgroup\$

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.