1
\$\begingroup\$

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
    },
\$\endgroup\$
4
  • \$\begingroup\$ Can you provide an example of how json_dict looks? I do not understand why you simply do not just check if "compensationsDeltaEmployee" is a valid key in the dict. if "compensationsDeltaEmployee" in json_dict,,, Is there something prohibiting this? \$\endgroup\$ Jan 27, 2022 at 22:24
  • \$\begingroup\$ I don't think there's anything that prohibits me from doing that I just wasn't thinking of it when I wrote the code. \$\endgroup\$
    – JThao
    Jan 27, 2022 at 22:39
  • \$\begingroup\$ read up on wiki.c2.com/?ArrowAntiPattern while I write up something =) \$\endgroup\$ Jan 27, 2022 at 22:42
  • 2
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Jan 28, 2022 at 8:32

1 Answer 1

6
\$\begingroup\$

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.

enter image description here

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)
\$\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.