CodeSOD: In the Know

Delilah works in a Python shop. Despite Python's "batteries included" design, that doesn't stop people from trying to make their own batteries from potatoes. For example, her co-worker wrote this function:

def key_exists(element, key):

    if isinstance(element, dict):

        try:

            element = element[key]

        except KeyError:

            return False

        return True

Python, of course, has an in operator. key in dictionary is an extremely common idiom. There's no reason to implement your own. Certainly, there's no reason to re-implement it by catching and throwing exceptions.

This is ugly, stupid, and bad. It gets worse, though, when you see how it gets used.

for key in old_yaml_data:

    if key in new_yaml_data:

        if old_yaml_data[key] != new_yaml_data[key]:

            temp = new_yaml_data[key]

            new_yaml_data[key] = merge(new_yaml_data[key], old_yaml_data[key])



            if key_exists(new_yaml_data[key], 'image') and key_exists(old_yaml_data[key], 'image'):

                new_yaml_data[key]['image'] = temp['image']

            elif key == "databases":

                revert_db_tags(new_yaml_data[key], temp)

This code is attempting to upgrade "old" YAML data with "new" data. So it's basically merging dictionaries, which is a great case for the in operator.

And they use the correct idiom on the second line there! This was written by one developer! They do the standard key in new_yaml_data check. And they also use key_exists. I can only assume that they had a stroke between starting and finishing this script, which I'll note is, in total, 48 lines long.

Here's the whole short script, which is just generally a mess. Slapped together Python code that's trying to be a "smarter" shell script, but is definitely written with the elegance of hacked-together-bash.

import sys

import yaml

from jsonmerge import merge



appHomePath = sys.argv[1]

oldValuesYAML = appHomePath + "values.yaml"

newValuesYAML = appHomePath + "/upgrade_version/values.yaml"

with open(newValuesYAML, 'r') as f:

    new_yaml_data = yaml.load(f, Loader=yaml.loader.FullLoader)

with open(oldValuesYAML, 'r') as f:

    old_yaml_data = yaml.load(f, Loader=yaml.loader.FullLoader)

def key_exists(element, key):

    if isinstance(element, dict):

        try:

            element = element[key]

        except KeyError:

            return False

        return True



def revert_db_tags(old_yaml_data, new_yaml_data):

    dbList = ["mongoDB", "postgresDB"]

    mongoDbTagsToRevert = ["mongoRestore"]

    mongodbKeysToDelete = []

    postgresDbTagsToRevert = []





    for db in dbList:

        old_yaml_data[db]['image'] = new_yaml_data[db]['image']

    for mongoDbTag in mongoDbTagsToRevert:

        old_yaml_data['mongoDB'][mongoDbTag]['image'] = new_yaml_data['mongoDB'][mongoDbTag]['image']

    for mongoDbTag in mongoKeysToDelete:

        del old_yaml_data['mongoDB'][mongoDbTag]



    for postgresDbTag in postgresDbTagsToRevert:

        old_yaml_data['postgresDB'][postgresDbTag]['image'] = new_yaml_data['postgresDB'][postgresDbTag]['image']



for key in old_yaml_data:

    if key in new_yaml_data:

        if old_yaml_data[key] != new_yaml_data[key]:

            temp = new_yaml_data[key]

            new_yaml_data[key] = merge(new_yaml_data[key], old_yaml_data[key])



            if key_exists(new_yaml_data[key], 'image') and key_exists(old_yaml_data[key], 'image'):

                new_yaml_data[key]['image'] = temp['image']

            elif key == "databases":

                revert_db_tags(new_yaml_data[key], temp)



with open(newValuesYAML, 'w') as f:

    data = yaml.dump(new_yaml_data, f, sort_keys=False)