Long time no blog. I’d like to break the ice with a lighthearted Python code readability optimization discussion.
I need a Python function that generates short, random, numeric codes of a specified length using a secure random number generator. The codes must not start with 0, but the output should be a string. This was my first draft:
import struct, os def make_code(digits): """Generate a random short code.""" if digits > 9: return make_code(9) + make_code(digits - 9) [rand] = struct.unpack('>L', os.urandom(4)) min_code = 10 ** (digits - 1) code = min_code + rand % (9 * min_code) return unicode(code)
Two things about this solution bothered me:
- The math I used looks too clever. I’m not sure future maintainers will be able to understand why it works.
- It discards a lot of the entropy provided by /dev/urandom, which could be expensive on some servers. (I confess this thought is premature optimization.)
To try to clean this up, I created a generator that produces random secure digits and based the make_code function on it:
def random_digits(): while True: [n] = struct.unpack('>L', os.urandom(4)) s = ('%d' % n)[1:] for c in s: yield c next_random_digit = random_digits().next def make_code(digits): return ''.join(next_random_digit() for _ in range(digits))
This felt better, but it has two critical problems. It can return 0 for the first digit, and due to the next_random_digit global, this solution is not thread safe! I’ve never tried to run a generator in multiple concurrent threads. I can only imagine the possible fireworks.
So I solved the problems as follows.
class RandomDigitStream(object): def __init__(self): self.streams = threading.local() def stream(self): while True: [n] = struct.unpack('>L', os.urandom(4)) s = ('%d' % n)[1:] for c in s: yield c def make_code(self, digits): """Generate a random short code containing only digits.""" try: next_digit = self.streams.next_digit except AttributeError: self.streams.next_digit = next_digit = self.stream().next while True: c = next_digit() # Don't use 0 for the first digit. if c != '0': break lst = [c] lst.extend(next_digit() for _ in range(1, digits)) return ''.join(lst) make_code = RandomDigitStream().make_code
Now the solution is large and I’m pretty sure it’s less readable than the original solution. Anyone want to take a shot at making this better? I’m currently thinking that the original solution was better than I thought.