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
    Jun 9 at 2:12
  • 1
    \$\begingroup\$ This code is incomplete (and won't run) without an except block. \$\endgroup\$
    – FMc
    Jun 9 at 5:07
  • \$\begingroup\$ Ahh sorry missed it on copy paste \$\endgroup\$
    – JThao
    Jun 9 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
    Jun 9 at 15:45

1 Answer 1

3
\$\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
    Jun 9 at 15:40
  • 2
    \$\begingroup\$ @JThao don't use Python 2. \$\endgroup\$
    – Reinderien
    Jun 9 at 15:43
  • \$\begingroup\$ I get that a lot but I don't have a choice in this matter :( \$\endgroup\$
    – JThao
    Jun 9 at 15:45
  • 1
    \$\begingroup\$ @JThao Edited; minor changes necessary. \$\endgroup\$
    – Reinderien
    Jun 9 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
    Jun 10 at 14:04

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.