Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I wrote a Python method to convert an object and a subset of its attributes into a JSON string with a given field as the key

There's a lot of string concatenation, and I'm not too sure what the best way to do that in Python is.

Also wondering if attr.replace('"','\"') is sufficient to sanitise input?

def to_json_with_key(nodes, key, fields, insertIndex=False):
    i=0
    s="{"
    for node in nodes:
        s += '"%s":{' % getattr(node,key)
        attrs=[]
        for field in fields:
            attr = getattr(node,field)
            if not isinstance(attr,int) and not isinstance(attr,float) and not isinstance(attr,long):
                attrs.insert(0, '"%s":"%s"' % (field, attr.replace('"','\"') ))
            else:
                attrs.insert(0, '"%s":%s' % (field, attr ))

        if (insertIndex):
            s+='index:%d,' % i
            i=i+1

        s+=','.join(attrs) + "},"
    s = s.rstrip(",") + "}"
    return s

Sample input:

to_json_with_key(myObj, "title", fields=["length","time_offset"], insertIndex=True)

Sample output:

{"Introduction_to_Lists":{index:0,"length":128,"time_offset":0,
 "Pass_by_Reference":{index:1,"length":84,"time_offset":128}}
share|improve this question

1 Answer

up vote 5 down vote accepted

Firstly, python includes a json module. Use that rather then writing your own.

However, just to give some comments on your style:

def to_json_with_key(nodes, key, fields, insertIndex=False):

python recommend words_with_underscores for parameter names

    i=0

Rather then manually keeping a count use enumerate

    s="{"
    for node in nodes:
        s += '"%s":{' % getattr(node,key)

You don't check the value to make sure it is json-safe. Building strings by concatenation is not usually a good idea. Either build a list and join it or use StringIO

        attrs=[]
        for field in fields:
            attr = getattr(node,field)
            if not isinstance(attr,int) and not isinstance(attr,float) and not isinstance(attr,long):

You can pass several types in a tuple to isinstance to check several types at once.

                attrs.insert(0, '"%s":"%s"' % (field, attr.replace('"','\"') ))

This won't be sufficient. For example, you could have a \ at the end of a string. You also need to handle newlines, tabs, and other escaped characters.

            else:
                attrs.insert(0, '"%s":%s' % (field, attr ))

Why insert rather then append?

        if (insertIndex):

No need for parens

            s+='index:%d,' % i
            i=i+1

        s+=','.join(attrs) + "},"
    s = s.rstrip(",") + "}"

It would be cleaner to join a list of items rather then stripping off stray commas. 

    return s
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.