Skip to main content
Clarify literal syntax.
Source Link
ferada
  • 11.1k
  • 25
  • 63
  • Everything could use more error checking. That is, check expected types and values instead of falling through the ifs ('imperial', 'metric', what happens if it's neither, or both?).
  • Unless you really prefer list and dict, it's better to use the literal syntax [] and {} (fast, can't override itthe names could be shadowed by some definition).
  • The type check for dicts can be better, i.e. isinstance(foo, dict); if you really want only dict, then it's still better to use type(foo) is dict instead of comparing strings. I'd also move that whole if/else into a function, e.g. maybe_load_json or so.
  • The value of document after the assignments isn't used anywhere, so it's safe to remove, though I'd rather use that variable again instead of introducing document2.
  • The second branch with 'small_encoded_polyline' seems to have the wrong level of indentation? At least it's different from the first one in that it's only run if the document wasn't a dict. Assuming that it's safe to run anyway I'll change that in the code below.
  • The pattern if foo in json and json[foo] is not None: could be easier be achieved with if json.get(foo) is not None: as the return value there defaults to None.
  • I'm not particularly fond of the habit to reuse input data like with 'points_correct' done here. It would be cleaner just to have a list points_correct and assign that to a jsonobject key (and you know, possibly put the calculation into a separate function as well).
  • Everything could use more error checking. That is, check expected types and values instead of falling through the ifs ('imperial', 'metric', what happens if it's neither, or both?).
  • Unless you really prefer list and dict, it's better to use the literal syntax (fast, can't override it).
  • The type check for dicts can be better, i.e. isinstance(foo, dict); if you really want only dict, then it's still better to use type(foo) is dict instead of comparing strings. I'd also move that whole if/else into a function, e.g. maybe_load_json or so.
  • The value of document after the assignments isn't used anywhere, so it's safe to remove, though I'd rather use that variable again instead of introducing document2.
  • The second branch with 'small_encoded_polyline' seems to have the wrong level of indentation? At least it's different from the first one in that it's only run if the document wasn't a dict. Assuming that it's safe to run anyway I'll change that in the code below.
  • The pattern if foo in json and json[foo] is not None: could be easier be achieved with if json.get(foo) is not None: as the return value there defaults to None.
  • I'm not particularly fond of the habit to reuse input data like with 'points_correct' done here. It would be cleaner just to have a list points_correct and assign that to a jsonobject key (and you know, possibly put the calculation into a separate function as well).
  • Everything could use more error checking. That is, check expected types and values instead of falling through the ifs ('imperial', 'metric', what happens if it's neither, or both?).
  • Unless you really prefer list and dict, it's better to use the literal syntax [] and {} (the names could be shadowed by some definition).
  • The type check for dicts can be better, i.e. isinstance(foo, dict); if you really want only dict, then it's still better to use type(foo) is dict instead of comparing strings. I'd also move that whole if/else into a function, e.g. maybe_load_json or so.
  • The value of document after the assignments isn't used anywhere, so it's safe to remove, though I'd rather use that variable again instead of introducing document2.
  • The second branch with 'small_encoded_polyline' seems to have the wrong level of indentation? At least it's different from the first one in that it's only run if the document wasn't a dict. Assuming that it's safe to run anyway I'll change that in the code below.
  • The pattern if foo in json and json[foo] is not None: could be easier be achieved with if json.get(foo) is not None: as the return value there defaults to None.
  • I'm not particularly fond of the habit to reuse input data like with 'points_correct' done here. It would be cleaner just to have a list points_correct and assign that to a jsonobject key (and you know, possibly put the calculation into a separate function as well).
added 3625 characters in body
Source Link
ferada
  • 11.1k
  • 25
  • 63
  1. I don't see why del jsonobject[...] would be necessary before reassigning reassigning that key. Also, the test in the last two lines could also also be simplified to something like    jsonobject.pop('small_encoded_polyline', None) if you don't care about about that key, no?

Also:

  • Everything could use more error checking. That is, check expected types and values instead of falling through the ifs ('imperial', 'metric', what happens if it's neither, or both?).
  • Unless you really prefer list and dict, it's better to use the literal syntax (fast, can't override it).
  • The type check for dicts can be better, i.e. isinstance(foo, dict); if you really want only dict, then it's still better to use type(foo) is dict instead of comparing strings. I'd also move that whole if/else into a function, e.g. maybe_load_json or so.
  • The value of document after the assignments isn't used anywhere, so it's safe to remove, though I'd rather use that variable again instead of introducing document2.
  • The second branch with 'small_encoded_polyline' seems to have the wrong level of indentation? At least it's different from the first one in that it's only run if the document wasn't a dict. Assuming that it's safe to run anyway I'll change that in the code below.
  • The pattern if foo in json and json[foo] is not None: could be easier be achieved with if json.get(foo) is not None: as the return value there defaults to None.
  • I'm not particularly fond of the habit to reuse input data like with 'points_correct' done here. It would be cleaner just to have a list points_correct and assign that to a jsonobject key (and you know, possibly put the calculation into a separate function as well).

I might have messed up some of the logic now, but I hope you get the idea:

def maybe_load_json(document):
    if isinstance(document, dict):
        return document
    return json.loads(document)


def cleaned_small_encoded_polylines(documents):
    result = []
    for document in documents:
        #sometimes these document objects turn out to be dicts
        #and sometimes they turn out to be strings
        #even though JSON output is always the same
        #is there a better way to deal with this?
        document = maybe_load_json(document)
        if document.get('small_encoded_polyline') is not None:
            del document['small_encoded_polyline']
            #I thought this line above would modify the original
            #jsonobject since document is a dictionary so I should
            #be working with a pointer to the original object
            #but inspection of jsonobject reveals this not to be the case
            result.append(document)
    return result

...

    r = requests.get(url, headers = {'User-Agent':UA})
    jsonobject = r.json()
    if jsonobject.get('laps') is not None:
        if 'imperial' in jsonobject['laps']:
            laps_array = jsonobject['laps']['imperial']
            type = 'imperial'
        if 'metric' in jsonobject['laps']:
            laps_array = jsonobject['laps']['metric']
            type = 'metric'
        if laps_array is not None:
            jsonobject['laps'][type] = cleaned_small_encoded_polylines(laps_array)

    # this can't be an elif because sometimes json objects
    # have both "points" and "laps"
    if jsonobject.get('points') is not None:
        if 'points' in jsonobject['points']:
            points_correct = []
            laps_array = jsonobject['points']['points']
            if laps_array is not None:
                jsonobject['points']['points'] = cleaned_small_encoded_polylines(laps_array)

    jsonobject.pop('small_encoded_polyline', None)
  1. I don't see why del jsonobject[...] would be necessary before reassigning that key. Also, the test in the last two lines could also be simplified to something like  jsonobject.pop('small_encoded_polyline', None) if you don't care about that key, no?
  1. I don't see why del jsonobject[...] would be necessary before reassigning that key. Also, the test in the last two lines could also be simplified to something like  jsonobject.pop('small_encoded_polyline', None) if you don't care about that key, no?

Also:

  • Everything could use more error checking. That is, check expected types and values instead of falling through the ifs ('imperial', 'metric', what happens if it's neither, or both?).
  • Unless you really prefer list and dict, it's better to use the literal syntax (fast, can't override it).
  • The type check for dicts can be better, i.e. isinstance(foo, dict); if you really want only dict, then it's still better to use type(foo) is dict instead of comparing strings. I'd also move that whole if/else into a function, e.g. maybe_load_json or so.
  • The value of document after the assignments isn't used anywhere, so it's safe to remove, though I'd rather use that variable again instead of introducing document2.
  • The second branch with 'small_encoded_polyline' seems to have the wrong level of indentation? At least it's different from the first one in that it's only run if the document wasn't a dict. Assuming that it's safe to run anyway I'll change that in the code below.
  • The pattern if foo in json and json[foo] is not None: could be easier be achieved with if json.get(foo) is not None: as the return value there defaults to None.
  • I'm not particularly fond of the habit to reuse input data like with 'points_correct' done here. It would be cleaner just to have a list points_correct and assign that to a jsonobject key (and you know, possibly put the calculation into a separate function as well).

I might have messed up some of the logic now, but I hope you get the idea:

def maybe_load_json(document):
    if isinstance(document, dict):
        return document
    return json.loads(document)


def cleaned_small_encoded_polylines(documents):
    result = []
    for document in documents:
        #sometimes these document objects turn out to be dicts
        #and sometimes they turn out to be strings
        #even though JSON output is always the same
        #is there a better way to deal with this?
        document = maybe_load_json(document)
        if document.get('small_encoded_polyline') is not None:
            del document['small_encoded_polyline']
            #I thought this line above would modify the original
            #jsonobject since document is a dictionary so I should
            #be working with a pointer to the original object
            #but inspection of jsonobject reveals this not to be the case
            result.append(document)
    return result

...

    r = requests.get(url, headers = {'User-Agent':UA})
    jsonobject = r.json()
    if jsonobject.get('laps') is not None:
        if 'imperial' in jsonobject['laps']:
            laps_array = jsonobject['laps']['imperial']
            type = 'imperial'
        if 'metric' in jsonobject['laps']:
            laps_array = jsonobject['laps']['metric']
            type = 'metric'
        if laps_array is not None:
            jsonobject['laps'][type] = cleaned_small_encoded_polylines(laps_array)

    # this can't be an elif because sometimes json objects
    # have both "points" and "laps"
    if jsonobject.get('points') is not None:
        if 'points' in jsonobject['points']:
            points_correct = []
            laps_array = jsonobject['points']['points']
            if laps_array is not None:
                jsonobject['points']['points'] = cleaned_small_encoded_polylines(laps_array)

    jsonobject.pop('small_encoded_polyline', None)
Source Link
ferada
  • 11.1k
  • 25
  • 63

  1. I don't see why del jsonobject[...] would be necessary before reassigning that key. Also, the test in the last two lines could also be simplified to something like jsonobject.pop('small_encoded_polyline', None) if you don't care about that key, no?