4
\$\begingroup\$

I have this specific scenario where I'm going to get a JSON that could have multiple Objects. From the Multiple Objects I will create a new Json that can have nested arrays growing by n amount depending on some json object values.

I hoping there is a better way to do this. Maybe my example below is the best way? I took a look at https://realpython.com/python-data-classes/ but I don't think that will help since it seems complicated to turn the dataclass object into another json.

Update: I think I will ingest the original json object into a dataclass. Then iterate over the objects.

Example Input: Note that 3 and 4 are break start and end times. Which is why you will see the start and end times show up in the break array.

[{
    "name": "Jay",
    "clock_type": 1,
    "start": "4am"
}, {
    "name": "Jay",
    "clock_type": 4,
    "start": "5am"
},
{
    "name": "Jay",
    "clock_type": 3,
    "end": "6am"
},
{
    "name": "Jay",
    "clock_type": 4,
    "start": "7am"
},
{
    "name": "Jay",
    "clock_type": 3,
    "end": "8am"
}]

Example output:

{
    "name": "Jay",
    "start": "4am",
    "break": [{
        "start": "5am",
        "end": "6am"
    }, {
        "start": "7am",
        "end": "8am"
    }]
}

What i'm doing right now is looping through the array, and building the punch dynamically based on the "clock_type".

import json

def write_new_json(data):

    request = {}
    break_count = 0

    for i in data:
        # 1 = New Object. First Clock In
        if i['clock_type'] in [1]:
            request.update({"name": i["name"],
                            "start": i["start"]})
        # 3 = Break End
        if i['clock_type'] in [3]:
            request["break"][break_count].update({"end": i["end"]})
            break_count += 1

        # 4 = Break start. If there is already a breaks object, then we do not create a breaks array, 
              # we just append to the existing one.
        if i['clock_type'] in [4]:
            if break_count == 0:
                request.update({"break": []})

            request["break"].append({"start": i["start"]})

       # 2 = Not in the example, but a punch of 2 would reset break_count to 0.


    return request


with open('example.json') as f:
    api_request = write_new_json(json.load(f))

print(json.dumps(api_request))
\$\endgroup\$
2
\$\begingroup\$

There are a few points in the existing code that I think could be improved.

Confusing function name write_new_json

Naming is important. Good names make your code more readable, and easier to understand. Bad names make it less readable. Names are bad when they don't describe what the thing is/does, or conversely, if they seem to describe something that the thing isn't/doesn't do.

Usually, the verbs "read" and "write" in programming refer to fetching data from, and sending data to, some external/system resource. Especially a file. So the name write_new_json() sounds like you're writing some JSON to a file. But that's not what the function is doing at all. It's actually transforming some JSON into some other kind of JSON. Therefore write_new_json() is a misleading name.

I would pick a new name that avoids write and probably even avoids json; describing the input and output formats of a function as "JSON" does nothing to capture the purpose of the function. How about build_clock_punches? build_time_logs?

Confusing variable name i

    for i in data:

Another naming nitpick. In most programming languages, it is conventional to name your loop counter variable i. In Python, this would be something like for i in range(0, 1000):.1 But in your case, i is not a loop counter; it is not even a number. Therefore the name is confusing. Try using a more descriptive name, e.g. for clock_punch in data: or for clock_event in data:.

Unnecessary use of in operator

        if i['clock_type'] in [3]:

You're using in because [3] is a list, but there doesn't seem to be any reason that [3] needs to be a list. You can just compare the value directly using the equality operator, e.g. if clock_event['clock_type'] == 3:.

Unnecessary variable break_count

As far as I can tell, you only use break_count to keep track of the length of request["break"]. Persistently tracking the length of a list in a separate variable is a bad idea: it is unnecessary, and it opens you up to bugs. Suppose you go in later and add a new line of code that changes the length of request["break"], but forget to add that second line of code that updates break_count? Or vice versa?

To find the length of a list directly, use len().

        if clock_event['clock_type'] == 3:
            last_break_index = len(request["break"]) - 1
            request["break"][last_break_index].update({"end": clock_event["end"]})

But wait! There's an even better approach here. Python offers a handy syntax for getting the last item in a list: my_list[-1].

        if clock_event['clock_type'] == 3:
            request["break"][-1].update({"end": clock_event["end"]})

And for the if break_count == 0: line, it is better to check for the non-existence of request["break"] directly.

        if clock_event['clock_type'] == 4:
            if "break" not in request:
                request.update({"break": []})

Too trusting

Your program assumes the input data will always be well-formed. What happens if it's not? Suppose the system that produces your input goes haywire, and you receive a break-start, and then two break-ends? Or receive a break-end without receiving any break-starts first? Or data with a mix of two or more names?2 Depending on the nature of the nonsense input, your program would either crash entirely, or produce a nonsense output of its own.

What your program should do in these cases is detect that something is wrong, and then throw some kind of exception that explains what the problem was. Unfortunately I don't have time to elaborate on every possible case you might encounter, so I'll have to leave that as an exercise for you.

Of course, if this is a school assignment or other non-real-world project where you have a guarantee that the data will be well-formed, you may not want to go through all the extra trouble. But it's something you should keep in mind for future projects.


Thoughts on using a data class

You mentioned wanting to use a data class to ingest the input data. If all you are planning to do is replace all the dicts in the list data with equivalent data class objects, and then run those objects through the same logic you have now...then you don't gain that much. You do get the type annotations, and there's nothing wrong with having those, but it's not a huge benefit.


1Technically, Python doesn't even have "loop counters" like some other languages do. All Python for-loops are foreach loops rather than "traditional" for-loops. But it's close enough that the conventions around i still apply.

2Actually, it seems perfectly reasonable for the upstream system to send you data for two or more names at once. You may want to enhance your code to support that (or not, I don't know your circumstances).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks! I really appreciate the input, especially the part about break counters! The data i'm getting is from some other code that I wrote. If it does not come in the format i'm expecting there was a pretty big issue since there is a lot in place to make sure it does. However, I've seen stranger things so I will definitely add some logic for what to do when the file is not as I expect it. \$\endgroup\$ – Philip Jay Fry Feb 21 at 15:07
  • \$\begingroup\$ @PhilipJayFry Well, if it's all one big program, then it's easier to trust that you're getting the right stuff. The best thing to do is ask "how much control do I have over what data the module receives?" and then work from there. \$\endgroup\$ – MJ713 Feb 21 at 18:12
  • \$\begingroup\$ @PhilipJayFry ...keeping in mind that code tends to get re-written, refactored, reused, and generally moved around. So even if you control the source of your data today, that doesn't guarantee that you will control it tomorrow. \$\endgroup\$ – MJ713 Feb 21 at 18:36
1
\$\begingroup\$

Three things to add to MJ713's answer:

For mutually exclusive options, use if ... elif .... When a match is found, the rest of the conditions are skipped. It may also help to put the order the tests so that the most common case is first and the least common case is last.

dict.setdefault(key, default_value) returns dict[key] if it is in the dict. Otherwise, it sets dict[key] to default_value and returns that.

Avoid so called "magic numbers". If someone else looks at your code, they won't know what 3 means. Indeed, you put a note in the problem description to say that 3 and 4 indicate break start/stop times. The numbers also make it more difficult to change the code (e.g., are you sure the 3 you are about to change is a break start and not just a 3?). It would be better to use a global constant or an enum.

Put together:

DAY_START = 1
BREAK_RESET = 2
BREAK_END = 3
BREAK_START = 4
DAY_END = 5

def collect_data(data):

    request = {}

    for clock_event in data:
        clock_type = clock_event['clock_type']

        if clock_type == BREAK_START:
            breaks = request.setdefault("break", [])
            breaks.append({"start": clock_event["start"]})

        elif clock_type == BREAK_END:
            request["break"][-1]["end"] = clock_event["end"]

        elif clock_type == DAY_START:
            request.update({"name": clock_event["name"],
                            "start": clock_event["start"]})

        # handle other clock types

        else:
            raise ValueError(f"Invalid 'clock_type': {clock_type}")    

    return request
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks! You make a very good point about my use of numbers in the logic flow. An Enum seems like a great alternative. \$\endgroup\$ – Philip Jay Fry Feb 21 at 15:09
  • \$\begingroup\$ All very good points. \$\endgroup\$ – MJ713 Feb 21 at 18:38

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.