JSON templates in Python

1. Introduction

This review grew to be very long, so I'll say up front that you shouldn't take the length of this to heart: your code is not bad, especially if you are new to Python. There's always a lot of things to say about a piece of code of this length, and idiomatic Python has a bunch of features (sets, generators, comprehensions, iterators) that will be unfamiliar to users of some other languages. So take everything I have to say with a pinch of salt (except for item #1 under "General comments", which is really the only big problem here).

2. General comments

1. By far the most important problem is your documentation. What is your library supposed to do? How do it use it? It is impossible to review code without understanding its purpose.

   As a user I expect to be able to write help(mockjson) to get a description of a module and help(mockjson.generate_json) to get a description of a function, but there's nothing here. Your online documentation is also unhelpful.

   After some hunting around I eventually found Mennon van Slooten's mockJSON documentation. I'm going to proceed on the basis that your purpose is to reimplement this in Python.

2. There's no test suite. I know it's hard for you to write test cases because of the randomness. You probably want to take a hint from Mennon van Slooten and provide a way for to change the random choices to a deterministic sequence of choices.

3. Your collections of example data (_male_first_name etc.) are stored as tuples, but it would be better to store these as lists. Tuples are fixed-size collections typically used as lightweight representations of records. Lists sets are variable-size collections typically containing similar items.

4. Your example data is very America-centric. Given that the purpose of your module is to produce example data for testing, it will be useful to have a wider variety of names, including names with accented letters, or in different scripts.

5. It is generally results in source code that is easier to read (with fewer quotation characters and commas) if you produce this kind of data using split. For example,

   _female_first_names = u"""
       Mary Patricia Linda Barbara Elizabeth
       Zoé Yến София فاطمة 明子 美玲 ยิ่งลักษณ์
       """.split()
   

6. The function _random_item is already in the Python library as random.choice.

7. The string "0123456789" is already in the Python library as string.digits.

8. It's not clear why _random_data needs to strip all initial @ signs from its argument before trying to look it up. You only call this function in a context where you could easily strip the @. It also fails to raise an error if the key is missing (so that mistakes like @NUBMER are not caught).

9. Your definitions of data will look nicer (fewer quotation marks) if you use the dict constructor:

   data = dict(
       NUMBER = lambda: _random_item("0123456789"),
       LETTER_UPPER = lambda: _random_item(string.uppercase),
       LETTER_LOWER = lambda: _random_item(string.lowercase),
       ...
   

10. Since most of the values in data are of the form lambda: _random_item(...), why not special-case them to save on boilerplate? You could write something like:

    def _random_data(key):
        """
        Construct a random data item for `key` and return it.
        Raise KeyError if `key` is unknown.
        """
        constructor = data[key]
        if isinstance(constructor, types.FunctionType):
            return constructor()
        else:
            return random.choice(constructor)
    

    and then:

    data = dict(
        NUMBER = string.digits,
        LETTER_UPPER = string.uppercase,
        LETTER_LOWER = string.lowercase,
        ...
    

11. Building a string by repeatedly extending it with += is a well-known anti-pattern in Python. This is because extending a string is inefficient in Python: a new string gets allocated and the old string copied across each time. This means that any algorithm that builds a string by repeated extension runs like O(n²). It is nearly always better to generate the items to be assembled into the string and then join them to produce the result. This technique also avoids fencepost errors like spurious extra spaces at the start or end. So your could write:

    def _lorem_ipsum():
        """
        Return a random paragraph of placeholder text.
        """
        length = random.randrange(2, len(_lorem) / 2)
        return ' '.join(random.choice(_lorem) for _ in xrange(length))
    

12. generate_json takes an optional argument name which is always ignored. You can omit this.

3. Comments on generate_json_object

1. The name could be improved: generate has a special meaning in Python (referring to the output of a generator), and the json part is misleading since there's nothing JSON-specific about this function (it operates on Python objects, not on JSON representations). Since instantiate is often used for filling in a template, I think I would use a name like instantiate_object. (And instantiate_json for generate_json.)

2. The variable length is misleadingly named because it's not always a length (it might just be a number). Perhaps n would be better. You also have to go through contortions because you are overloading its meaning: it's None if no number was specified (in which case sometimes it needs to be treated as if it has the value 1), or else it's a number. It would be much clearer to have a separate variable (have_n in my code below) that indicates whether a number was supplied.

3. Your regular expression for finding repeat counts in JSON keys is \w+\|(\d+)-(\d+). This matches strings like "a|0-2trailing garbage". It's probably a good idea to anchor it to the end of the string (\w\|(\d+)-(\d+)$). Or maybe, depending on exactly what you want to match, to the beginning of the string as well: re.match(r'\w+\|(\d+)-(\d+)$', name). But then you need to decide what to do for non-matching keys. Maybe raise an exception?

4. The structure

   matches = re.search(...)
   if matches:
        ...
   

   is so common that readers will know what you mean if you abbreviate matches to m.

5. matches.group(0) is simpler than matches.groups()[0].

6. You perform three regular expression matches on the key: first, to find the repeat counts, second, to strip all suffixes, and third, to find the increment. If you refactor the function to take the repeat count as an argument (instead of the name), you can do these three matches all at the same time, like this:

   m = re.match(r"^(\w+)(?:\|(?:(\d+)-(\d+)|\+(\d+)))?$", '', key)
   if not m:
       raise ValueError("Bad key: {0}".format(key))
   have_n = False
   n = 1
   if m.group(2):
       have_n = True
       n = random.randint(int(m.group(2)), int(m.group(3)))
   increment = 0
   if m.group(4):
       increment = int(m.group(4))
   

   Now this doesn't quite work as I've written it above because of the way you've structured the function. Having used your regular expression to parsed the number and increment out of the key, you then pass the name when you make your recursive call, which means that the number has to be parsed again. To do this, you need to restructure the function so that instead of passing name to the recursive call, you pass have_n, n and increment. See later on where I give the whole text of the function as revised.

7. It is usually better in Python to test if an object belongs to a type t by writing isinstance(object, t) rather than type(object) is t. The reason is that object may belong to a subtype of t (for example, a defaultdict or OrderedDict instead of a plain dict).

   There is a gotcha here, which is that bool is a subtype of int, so you need to order your tests so that Booleans get tested before integers.

8. The structure of generate_json_object involves various branches which assign an object to the variable generated. This is then returned at the end. Why not just return the object directly and avoid the local variable?

9. When you're building a list, you do this by repeatedly calling append. This is not quite as bad as repeatedly calling += on a string (lists are a bit more flexible), but you can simplify this code by using a list comprehension:

   elif isinstance(template, list):
       return [generate_json_object(template[0]) for _ in xrange(length)]
   

   Note that when the loop variable is ignored (as here) it's conventional in Python to name it _.

10. When you handle an incremented value, you write template[key] += increment which updates the template permanently. That's all very well if you are going to use the template just once, but what if you want to use the same template many times?

    In order to allow re-use of templates, you are going to have to store the current increment for each template key in a separate data structure from the template, so that you can throw away this data structure when your instantiation is complete. The way to do this is to use a dictionary whose keys are locations in the template and whose values are the current increment for that location.

    Now, how can we represent a location in a template in such a way that we can use it as a key in a dictionary? Well, a location is a template is always a key in a dictionary, so we'd like to represent it as the pair (dictionary, key). However, this won't work, because only immutable (constant) objects can be used as keys in dictionaries in Python, and the template (being a dictionary) is mutable. But Python provides the function id which returns a unique identifier for each object in memory, and we can use that instead:

    increment = 0
    template_key = (id(template), key)
    if template_key in _increments:
        increment = _increments[template_key]
    elif m.group(4):
        increment = int(m.group(4))
        _increments[template_key] = increment
    

    Where should this _increments dictionary be stored? We need a new one for each instantiation we do, but it needs to persist throughout the recursive series of calls. We could pass it around as another function parameter, but if you find yourself doing this, that's a sign that you need to use a class. Again, see the final version at the end for how this can be done.

    The code I gave above isn't quite right, because although it remembers the increment, it doesn't actually update it each time it is used. Python has a very neat way to make this happen: iterators. Instead of storing increment, we'll construct an iterator that yields the values 0, increment, 2*increment, 3*increment, and so on. The function itertools.count does this job. And when there's no increment, we'll construct an iterator that always yields the value 0 no matter how many times we call it (using itertools.repeat):

    increment = itertools.repeat(0)
    if template_key in self._increments:
        increment = self._increments[template_key]
    elif m.group(4):
        increment = itertools.count(start = 0, step = int(m.group(4)))
        self._increments[template_key] = increment
    

4. Revised instantiation code

OK, let's put all that together. (There are a couple of bonus improvements in here for you to spot for yourself!)

_constantly_zero = itertools.repeat(0)

class _Instantiator(object):
    def __init__(self):
        self._increments = dict()

    def instantiate(self, template, have_n = False, n = 1,
                    increment = _constantly_zero):
        if isinstance(template, dict):
            generated = {}
            for key, value in template.iteritems():
                m = re.match(r"^(\w+)(?:\|(?:(\d+)-(\d+)|\+(\d+)))?$", key)
                if not m:
                    raise ValueError("Bad key: {0}".format(key))
                have_n = False
                n = 1
                if m.group(2):
                    have_n = True
                    n = random.randint(int(m.group(2)), int(m.group(3)))
                template_key = (id(template), key)
                increment = _constantly_zero
                if template_key in self._increments:
                    increment = self._increments[template_key]
                elif m.group(4):
                    increment = itertools.count(start = 0, step = int(m.group(4)))
                    self._increments[template_key] = increment
                generated[m.group(1)] = self.instantiate(value, have_n, n, increment)
            return generated
        elif isinstance(template, list):
            return [self.instantiate(template[0]) for _ in xrange(n)]
        elif isinstance(template, bool):
            if have_n:
                return bool(n)
            else:
                return template
        elif isinstance(template, int):
            if have_n:
                return n
            else:
                return template + next(increment)
        elif isinstance(template, str) or isinstance(template, unicode):
            if template:
                pattern = re.compile(r'@([A-Z_0-9]+)')
                repl = lambda m: _random_data(m.group(1))
                return ''.join(pattern.sub(repl, template) for _ in xrange(n))
            else:
                return ''.join(random.choice(string.letters) for _ in xrange(n))
        else:
            return template

def instantiate_object(template):
    """
    Instantiate an object based on `template` and return it.
    """
    return _Instantiator().instantiate(template)