0
\$\begingroup\$

As you can see I am using a counter variable to iterate through my JSON file and it's a dictionary that has a list of dictionaries. Once I find the key normalHours I divide it and add it to my dictionary hours_per_week. Then I update the dictionary it is associated with. Is it better to use for i in range instead of the counter? Anything else I can do to clean up or speed up the code?

Here is my json file, I deleted most of the key pairs for easier readability.

{ "employeeData": [ { "normalHours": 80, "givenName": "ROBERTO", }, { "normalHours": 80, "givenName": "HEATHER", }, { "normalHours": 80, "givenName": "PAUL", } ] }

def add_hours_per_week(json_file):
    """
    Add new dictionary value hoursPerWeek to our JSON data
    """
    # Create new dictionary hours_per_week.
    hours_per_week = {}
    hours_divide_by_2 = 0
    normal_hours_counter = 0

    try:
        for key, values in json_file.items():
            if str(key) == "employeeData":
                # JSON is in a list of dictionaries
                for item in values:
                    # If normalHours field is Null, "", or 0 add an empty field
                    if not item["normalHours"] or item["normalHours"] == 0:
                        hours_per_week["hoursPerWeek"] = ""
                    else:
                        # Divide normalHours by 2 rounded to 2 decimals.
                        hours_divide_by_2 = round(float(item["normalHours"]) / 2, 2)
                        hours_per_week["hoursPerWeek"] = hours_divide_by_2
                    json_file['employeeData'][normal_hours_counter].update(hours_per_week)
                    normal_hours_counter += 1
        print("%d employee updated with hoursPerWeek" % normal_hours_counter if normal_hours_counter <= 1 else
              "%d employees updated with hoursPerWeek" % normal_hours_counter)
        return json_file

    except Exception as e:
        print("Exception in add_hours_per_week: ", e)
\$\endgroup\$
4
  • 1
    \$\begingroup\$ But what does your code do? The standard on this site is to make that the title. And explain that a bit in the body. See How to Ask and help center. \$\endgroup\$
    – Teepeemm
    Commented Jun 9, 2022 at 2:12
  • 1
    \$\begingroup\$ This code is incomplete (and won't run) without an except block. \$\endgroup\$
    – FMc
    Commented Jun 9, 2022 at 5:07
  • \$\begingroup\$ Ahh sorry missed it on copy paste \$\endgroup\$
    – JThao
    Commented Jun 9, 2022 at 13:51
  • \$\begingroup\$ My answer missed the fact that you're using Python 2 so I'll update it. But why are you doing so? \$\endgroup\$
    – Reinderien
    Commented Jun 9, 2022 at 15:45

1 Answer 1

4
\$\begingroup\$

This code exhibits a collection of anti-patterns typically seen in Python that together are making your life more difficult:

  • Dictionary lasagna, having non-structured data used directly from the result of a JSON parse
  • In-place mutation
  • Exception swallowing
  • Infectious nullity

I don't see any value in your employees updated print. At the absolute least, this should be converted to a proper logging call, but I would sooner drop it entirely.

This code needs to be significantly re-thought, along with any code you haven't shown that depends on such patterns. Consider instead:

  • Define a simple dataclass or NamedTuple representing each Employee
  • Write a simple property method for hours_per_week
  • Do not round() in your business logic, only upon presentation
  • Explicit casting to a float should not be needed

Suggested

from typing import Any, Iterable, Iterator, NamedTuple, Optional


class Employee(NamedTuple):
    given_name: str
    normal_hours: Optional[float]

    @classmethod
    def from_dict(cls, data: Iterable[dict[str, Any]]) -> Iterator['Employee']:
        for employee in data:
            yield cls(
                given_name=employee['givenName'],
                normal_hours=employee.get('normalHours'),
            )

    @property
    def hours_per_week(self) -> Optional[float]:
        return self.normal_hours and self.normal_hours / 2

    def __str__(self) -> str:
        if self.normal_hours:
            return (
                f'{self.given_name}:'
                f' {self.normal_hours} hours,'
                f' {self.hours_per_week:.2f} hours per week'
            )
        return self.given_name


def main() -> None:
    json_content = {
        'employeeData': [
            {'normalHours':   80, 'givenName': 'ROBERTO'},
            {'normalHours':   80, 'givenName': 'HEATHER'},
            {'normalHours':   80, 'givenName': 'PAUL'},
            {'normalHours': None, 'givenName': 'JULIO'}
        ]
    }
    employees = Employee.from_dict(json_content['employeeData'])

    for employee in employees:
        print(employee)


if __name__ == '__main__':
    main()

Output

ROBERTO: 80 hours, 40.00 hours per week
HEATHER: 80 hours, 40.00 hours per week
PAUL: 80 hours, 40.00 hours per week
JULIO

Python 2

Similar, just worse.

class Employee:
    def __init__(self, given_name, normal_hours):
        self.given_name = given_name
        self.normal_hours = normal_hours

    @classmethod
    def from_dict(cls, data):
        for employee in data:
            yield cls(
                given_name=employee['givenName'],
                normal_hours=employee.get('normalHours'),
            )

    @property
    def hours_per_week(self):
        return self.normal_hours and self.normal_hours / 2

    def __str__(self):
        if self.normal_hours:
            return '%s: %f hours, %.2f hours per week' % (
                self.given_name,
                self.normal_hours,
                self.hours_per_week,
            )
        return self.given_name


def main():
    json_content = {
        'employeeData': [
            {'normalHours':   80, 'givenName': 'ROBERTO'},
            {'normalHours':   80, 'givenName': 'HEATHER'},
            {'normalHours':   80, 'givenName': 'PAUL'},
            {'normalHours': None, 'givenName': 'JULIO'}
        ]
    }
    employees = Employee.from_dict(json_content['employeeData'])

    for employee in employees:
        print(employee)


if __name__ == '__main__':
    main()
\$\endgroup\$
8
  • \$\begingroup\$ I'm using Python 2, in def str how would the format work? \$\endgroup\$
    – JThao
    Commented Jun 9, 2022 at 15:40
  • 2
    \$\begingroup\$ @JThao don't use Python 2. \$\endgroup\$
    – Reinderien
    Commented Jun 9, 2022 at 15:43
  • \$\begingroup\$ I get that a lot but I don't have a choice in this matter :( \$\endgroup\$
    – JThao
    Commented Jun 9, 2022 at 15:45
  • 1
    \$\begingroup\$ @JThao Edited; minor changes necessary. \$\endgroup\$
    – Reinderien
    Commented Jun 9, 2022 at 20:38
  • 1
    \$\begingroup\$ First, if your class has 50 more fields/attributes, it's possible that that's needed and appropriate, but also possible that that's a data schema misrepresentation and/or a normalisation error. Impossible to say without seeing it. \$\endgroup\$
    – Reinderien
    Commented Jun 10, 2022 at 14:04

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.