What is the pythonic way to update my nested dictionary?

Question

So I need to update my nested dictionary where the key is "compensationsDeltaEmployee". I created a string cost_perhour to hold the value of my conditional statements. Now that I am done with the string. What is the best or pythonic way to update my nested dictionary with a new key "costPerHour" with the value of my string cost_perhour? What I did was I created an empty dictionary cost_per_hour_dic then add the string then ran update. Is this okay or can I clean it up more?

def add_cost_per_hour(json_dic):
    """
    Add new dictionary value costPerHour to our data
    """
    
    cost_perhour = ""
    cost_per_hour_dic = {}
    try:
        # Find key compensationsDeltaEmployee.
        for keys, values in json_dic.items():
            if str(keys) == "compensationsDeltaEmployee":
                if "payBasis" in values:
                    # If payBasis equal 9, 0, P, cost_per_hour field should be blank.
                    if str(values["payBasis"]) in ("9", "0", "P"):
                        cost_perhour = ""

                    # If payBasis equal 1, A, B, C, D, H, J, 3, cost_per_hour equals salaryPayable divide by 2080.
                    elif str(values["payBasis"]) in ("1", "A", "B", "C", "D", "H", "J", "3"):
                        # Check if our value for salaryPayable is empty or None
                        if values["salaryPayable"] == "" or values["salaryPayable"] is None:
                            raise Exception("salaryPayable field is empty")
                        else:
                            cost_perhour = round(float(values["salaryPayable"]) / 2080, 2)

                    # If payBasis equal 2, 4, 5, 7, E, F, X, cost_per_hour should match the salaryPayable field.
                    elif str(values["payBasis"]) in ("2", "4", "5", "7", "E", "F", "X"):
                        if values["salaryPayable"] == "" or values["salaryPayable"] is None:
                            raise Exception("salaryPayable field is empty")
                        else:
                            cost_perhour = round(float(values["salaryPayable"]), 2)

                    # If there are any unexpected values, the cost_per_hour field should be blank.
                    else:
                        cost_perhour = ""
                else:
                    raise Exception("Could not find key payBasis")
        if cost_perhour is "":
            raise Exception("cost_per_hour is empty")

        cost_per_hour_dic["costPerHour"] = str(cost_perhour)
        json_dic["compensationsDeltaEmployee"].update(cost_per_hour_dic)

        return json_dic

    except Exception as e:
        print("Exception in add_cost_per_hour: ", e)

The json_dict

"compensationsDeltaEmployee": {
    "interPersonnelAgree": "N",
    "payRateDeterminant": "0",
    "payPlan": "1",
    "properPayPlan": null,
    "retainedGrade": null,
    "payBasis": "2",
    "gradeCode": "06",
    "step": "03",
    "basePayChangeYypp": null,
    "physicalCompAllowance": 0,
    "withinGradeEligibilityCode": "1",
    "salaryPayable": 21
    },

Answer 1

I might be a bit too harsh, so feel free to read this review in small chunks. What is the number one thing that stands out when I read this code? It feels like it has been written in the spur of the moment, little thought have been given to the overall structure. Feautures seems to been added as needed, instead of taking a step back to look if anything is redundant.

I can not recommend the following strongly enough

Trace out the program structure on paper before you begin

It can be a very rough sketch, but you need to imagine the flow of your program before you start. Imagine if Frodo and Bilbo had just started walking to mordor without a plan, or if the people building rockets at NASA just said YOLO? Be consious of your own code.

JSON should not be used as an internal python datastructure

I am lazy, but it is much clearer storing objects in python as classes. Dicts and especially json are great for reading data to and from python, but internally I recommend sticking for classes in this case.

Avoid falling into the anti-arrow pattern

Reading deeply nested code is difficult and hard to maintain, it should give you a signal that you need to refactor the code.

Avoid Bare except

The first rule of thumb is to absolutely avoid using bare except, as it doesn't give us any exceptions object to inspect.

Furthermore, using bare except also catches all exceptions, including exceptions that we generally don’t want, such as SystemExit or KeyboardInterrupt.

Catching every exception could cause our application to fail without us really knowing why. This is a horrible idea when it comes to debugging.

Stop Using raise Exception

Secondly, we should avoid raising a generic Exception in Python because it tends to hide bugs.

Replace nested conditionals with guard clauses

See for instance here for a longer explanation

for keys, values in json_dic.items():
    if str(keys) == "compensationsDeltaEmployee":
        # More code here

Is better expressed as

compensation = json_dic.get("compensationsDeltaEmployee")
if compension is None:
    break / return / raise specific error
# More code here

Don't repeat yourself

                # If payBasis equal 9, 0, P, cost_per_hour field should be blank.
                if str(values["payBasis"]) in ("9", "0", "P"):
                    cost_perhour = ""

This breaks the DRY principle several times.

• Do not comment the obvious
• Do not comment what the code is doing
• Comment why you are doing it.

Secondly cost_perhour is already set to "" so this entire block is redundant.This followng block is repeated twice when it is not needed

                    if values["salaryPayable"] == "" or values["salaryPayable"] is None:
                        raise Exception("salaryPayable field is empty")
                    else:
                        cost_perhour = round(float(values["salaryPayable"]), 2)

What you are doing is first checking if should be x / 2080, then in the next clause you are checking if it should be x. Why not just check if it should be x, and if it should be x, then check if we should divide?

A more sensible name for the dict, could be EMPLOYEES, but really it should be a descriptive name. Tell me what it is a dictionary of, instead of telling me it is a generic dictionary

EMPLOYEES = {
    "compensationsDeltaEmployee": {
        "interPersonnelAgree": "N",
        "payRateDeterminant": "0",
        "payPlan": "1",
        "properPayPlan": None,
        "retainedGrade": None,
        "payBasis": "2",
        "gradeCode": "06",
        "step": "03",
        "basePayChangeYypp": None,
        "physicalCompAllowance": 0,
        "withinGradeEligibilityCode": "1",
        "salaryPayable": 21,
    },
}

Using all the tricks int book the code windless down into this

EMPLYEE_COMPENSATION = "compensationsDeltaEmployee"
COST_PER_HOUR_DIVIDE_BY_CONSTANT = {"1", "A", "B", "C", "D", "H", "J", "3"}
DIVIDE_COST_BY_HOUR_CONSTANT = 2080
COST_PER_HOUR_EQUALS_SALARY_PAYABLE = COST_PER_HOUR_DIVIDE_BY_CONSTANT.union(
    {"2", "4", "5", "7", "E", "F", "X"}
)


def cost_per_hour(compensation):
    """Calculates the cost per hour from the compensationsDeltaEmplyee field"""

    pay_basis = str(compensation["payBasis"])
    if not pay_basis in COST_PER_HOUR_EQUALS_SALARY_PAYABLE:
        return ""

    cost_perhour = float(compensation["salaryPayable"])
    if pay_basis in COST_PER_HOUR_DIVIDE_BY_CONSTANT:
        cost_perhour /= DIVIDE_COST_BY_HOUR_CONSTANT

    return round(cost_perhour, 2)


def update_cost_per_hour(employees):
    compensation = employees.get(EMPLYEE_COMPENSATION)
    if compensation is None:
        raise KeyError("Could not find", EMPLYEE_COMPENSATION)

    cost = cost_per_hour(compensation)
    if not cost:
        raise ValueError("cost_per_hour is empty")

    employees[EMPLYEE_COMPENSATION]["costPerHour"] = cost
    return employees


if __name__ == "__main__":
    employees = update_cost_per_hour(EMPLOYEES)
    print(employees)