parsing json file with potential corrupted record and sorting by id to output first n lines - cleaner logic

Question

messy json file is parsed and should output the N highest record IDs & scores by score in descending order, highest score first.

the code works but i wonder if there is better readability, help me refactor it.

The function takes 2 args - the data file path and num of scores to return:

python myprogram.py highest score_recs.data 25

The input data file has a record per line. Each line has the following structure:

<score>: <json string>

If the line has a score that would make it part of the highest scores, then the remainder of the line must be parsable as JSON, and there must be an id key at the top level of this JSON doc.

An example data file could look like this:

10622876: {"umbrella": 99180, "name": "24490249af01e145437f2f64d5ddb9c04463c033", "value": 12354, "payload": "........", "date_stamp": 58874, "time": 615416, "id": "3c867674494e4a7aac9247a9d9a2179c"}
13214012: {"umbrella": 924902, "name": "70dd4d9494d1cd0362e123ce90f4053726b29e97", "value": 976852, "payload": "........", "date_stamp": 3255, "time": 156309, "id": "085a11e1b82b441184f4a193a3c9a13c"}
11446512: {"umbrella": 727371, "name": "8e21427b2350023079835361dce03cdea95a2983", "value": 70801, "payload": "........", "date_stamp": 1730, "time": 496866, "id": "84a0ccfec7d1475b8bfcae1945aea8f0"}
11025835: === THIS IS NOT JSON and should error
11269569: {"umbrella": 902167, "name": "e4898b9bf79831cf36811917a797ef0fcf3af636", "value": 593180, "payload": "........", "date_stamp": 58736, "time": 1014495, "id": "7ec85fe3aa3c4dd599e23111e7abf5c1"}
11027069: {"umbrella": 990975, "name": "8aa306fb59e275a7e39debb1d5113ff411df22ad", "value": 67842, "payload": "........", "date_stamp": 60161, "time": 225413, "id": "f812d487de244023a6a713e496a8427d"}

initially json file is not sorted by scores.

steps:

• the bad json error is thrown only if there is bad record within the first minimum n unsorted lines,i.e. in the original json
• otherwise, even if there is bad record but not within the first n unsorted lines i can keep all the records and parse it
• sort it by scores in desc order, then keep the first n output a list of dicts, each dict has 2 fields - score,id

For example, if run with an N of 3 and 4th entry is bad json, it still works and produce:

python myprogram highest score_recs.data 3

[
    {
        "score": 13214012,
        "id": "085a11e1b82b441184f4a193a3c9a13c"
    },
    {
        "score": 11446512,
        "id": "84a0ccfec7d1475b8bfcae1945aea8f0"
    },
    {
        "score": 11269569,
        "id": "7ec85fe3aa3c4dd599e23111e7abf5c1"
    }
]

But when run with an N that includes that line, it would error:

$ python myprogram highest score_recs.data 10

invalid json format No JSON object could be decoded
 THIS IS NOT JSON

The scores are unique across the data set * Scores can repeat, but you should only count the id of the last line processed as the “winning” id. if score is not present it should give format error.

scores.data:

10622876: {"umbrella": 99180, "name": "24490249af01e145437f2f64d5ddb9c04463c033", "value": 12354, "payload": "........", "date_stamp": 58874, "time": 615416, "id": "3c867674494e4a7aac9247a9d9a2179c"}
13214012: {"umbrella": 924902, "name": "70dd4d9494d1cd0362e123ce90f4053726b29e97", "value": 976852, "payload": "........", "date_stamp": 3255, "time": 156309, "id": "085a11e1b82b441184f4a193a3c9a13c"}
11446512: {"umbrella": 727371, "name": "8e21427b2350023079835361dce03cdea95a2983", "value": 70801, "payload": "........", "date_stamp": 1730, "time": 496866, "id": "84a0ccfec7d1475b8bfcae1945aea8f0"}
11025835: === THIS IS NOT JSON and should error if this line is part of the result set, but is ok if it not ==
11269569: {"umbrella": 902167, "name": "e4898b9bf79831cf36811917a797ef0fcf3af636", "value": 593180, "payload": "........", "date_stamp": 58736, "time": 1014495, "id": "7ec85fe3aa3c4dd599e23111e7abf5c1"}
11027069: {"umbrella": 990975, "name": "8aa306fb59e275a7e39debb1d5113ff411df22ad", "value": 67842, "payload": "........", "date_stamp": 60161, "time": 225413, "id": "f812d487de244023a6a713e496a8427d"}

the python code:

import json
import sys

def main(filename, n):
    res = []

    main = dict()
    cnt = 1
    with open(filename, "r") as f:
        for line in f:
            id, content = line.split(":", 1)

            try:
                id = int(id)
            except ValueError:
                raise "Format error - no id"

            if "IS NOT JSON" in content:
                print("found bad json - check if we can continue")
                if cnt <= n:
                    # here this means the original unsorted json file has bad json within the first needed n lines - so we need to throw error as per the condition
                    raise "ERROR: THIS IS NOT JSON"
                else:
                    print("ah no, its good")

            else:
                try:  # need try here cuz when json.loads() sees bad json - it will throw jsondecode error, that i need to catch
                    newd = json.loads(content)
                    main[id] = newd
                    cnt += 1
                except:
                    print(
                        "keep pushing! that bad json is after the minimum n we need - so we need to record all jsons then sort then get the first n"
                    )

    main = [[k, v] for k, v in main.items()]
    main = sorted(main, reverse=True, key=lambda x: x[0])

    #get the first sorted n items, create a dict [score][id]
    main = dict(main[:n])

    for k, v in main.items():
        score = k
        id = v["id"]
        newd = dict()
        newd["score"] = score
        newd["id"] = id

        res.append(newd)    

    return res
    
arg_list = (sys.argv)
    
filenamepath = str(arg_list[1])
num_lines = int(arg_list[2])
    
main(filenamepath, num_lines)

Answer 1

name your module

$ python myprogram.py ...

Yeah, that's not a name.

Maybe high_score_report.py ?

name your function

def main...

This is a terrific name for "first function that will execute". But its body is entirely too long -- it contains all of your business logic. If you retain the name main, then this function should pretty much immediately delegate to better named code whose name describes the logic.

don't rename

def main(filename, n):
    res = []

    main = dict()

Uggh! First main is a module-level function, then we shadow that with a local variable. No. Don't do that. Pick a new name. It doesn't matter to the machine, but it does matter to the maintenance engineer who is reading this code months down the road. Especially if the engineer has a phone conversation about the code.

Similarly, further down in your code, prefer an identifier of id_ rather than shadowing the builtin id.

---

crack argv

arg_list = (sys.argv)

The extra ( ) parens are weird and distracting. Please format with black to remove such artifacts.

Also, renaming the familiar sys.argv doesn't seem to help humans much.

use an if __name__ guard

arg_list = (sys.argv)
    
filenamepath = str(arg_list[1])
num_lines = int(arg_list[2])
    
main(filenamepath, num_lines)

This will run when other modules, such as your unit tests, import this module. Use a guard to prevent such side effects.

Also this is too long, and not just because the str() is redundant. Rewrite with slice + tuple unpack:

def main(filename, n):
    ...
if __name__ == "__main__":`
    filenamepath, num_lines = sys.argv[1:]
    main(filenamepath, int(num_lines))

Or better, add type annotation and let typer worry about cracking argv:

from pathlib import Path
import typer

def main(filename: Path, n: int):
    ...
if __name__ == "__main__":`
    typer.run(main)

---

do one thing well

Each function should have a single responsibility. Here, you appear to be doing several things:

1. Identify well-formed lines that start with integer + colon.
2. Identify "bad JSON" lines, distinguishing between "late bad" and fatal "early bad".
3. Report on just the top-N entries.

In a bash script we might organize this as a multi-step pipeline using the | pipe character. In python it is often convenient to chain generators together. So we might have:

from operator import itemgetter
    ...
    def get_scored_content(self):
        with open(self.filename) as f:
            for line in f:
                score, content = line.split(":", 1)
                yield int(score), content

    def get_json_ids(self):
        for i, (score, content) in enumerate(self.get_scored_content()):
            if i < self.n:
                d = json.loads(content)
            else:
                try:
                    d = json.loads(content)
                except json.decoder.JSONDecodeError:
                    continue
            yield score, d["id"]

    def get_top_entries(self):
        return sorted(self.get_json_ids(), reverse=True, key=itemgetter(0))[:self.n]

Now a simple comprehension can easily turn those N entries into the desired list of dicts.

Note the use of the common itemgetter idiom.

use accurate identifiers

You wrote id when representing a score. I renamed it in the above code.

---

display results

The OP code evaluates main for side effects and discards its dict result. You probably wanted to send the result to stdout or to a file.

---

This module appears to achieve many of its design goals. It has accumulated some technical debt and would benefit from a bit of refactoring.

I would be reluctant to delegate maintenance tasks on this codebase in its current form.