3
\$\begingroup\$

This is a complete rewrite of the code posted for review in this question. The question is identical.

def render(digits):

    '''This function converts its input, a string of decimal digits, into a
    barcode, using the interleaved 2 of 5 format. The input string must not
    contain an odd number of digits. The output is an SVG string.

    Wikipedia [ITF Format]: http://en.wikipedia.org/wiki/Interleaved_2_of_5
    '''

    top = '<svg height="58" width="{0}" style="background:white">'
    bar = '<rect x="{0}" y="4" width="{1}" height="50" style="fill:black"/>'
    barcode = [top.format(len(digits) * 14 + 24)]

    byte_map = {
        '0': (2, 2, 4, 4, 2), '1': (4, 2, 2, 2, 4),
        '2': (2, 4, 2, 2, 4), '3': (4, 4, 2, 2, 2),
        '4': (2, 2, 4, 2, 4), '5': (4, 2, 4, 2, 2),
        '6': (2, 4, 4, 2, 2), '7': (2, 2, 2, 4, 4),
        '8': (4, 2, 2, 4, 2), '9': (2, 4, 2, 4, 2)
        }

    byte = lambda i: byte_map[digits[i]]

    def encode(bits, i=0):

        '''Encodes a string of decimal digits. The output's a binary string,
        but as a list of ints, all 2s and 4s, rather than zeros and ones, as
        the bars in the SVG barcode are rendered 2px wide for zeros, 4px for
        ones.'''

        if i == len(digits): return bits

        for b, w in zip(byte(i), byte(i+1)): bits.extend([b, w])

        return encode(bits, i+2)

    def svg(bits, i=0, x=4):

        '''Converts the list of ints returned by `encode` to a list of SVG
        elements that can be joined to form an SVG string.'''

        if i == len(bits): return barcode + ['</svg>']

        b, w = bits[i:i+2]
        barcode.append(bar.format(x, b))

        return svg(bits, i+2, x+b+w)

    # The call to `encode` passes in the itf start code, and the itf end
    # code's added to the list that `encode` returns. The complete list
    # is passed to `svg`, which returns a list of SVG element strings
    # that're joined with newlines to create the actual SVG output.

    return '\n'.join(svg(encode([2, 2, 2, 2]) + [4, 2, 2, 0]))

The use if recursive functions to implement what appear to be simple iterators is thought out. As a simple win, it avoids the need to try and assign xrange to range for Python2, as we don't need the range function now. Further, neither function can be replaced with list or generator expressions, so you end up with a couple of fairly crufty loops and more temp vars.

\$\endgroup\$
1
\$\begingroup\$

Normally I would just make this a comment, but I don't have enough rep here yet to do so. I haven't really done any Python at all, however this did catch my attention:

The input string must not contain an odd number of digits.

But there's no check to see if the input length is odd in the code. Surely you should throw an exception or at the very least return a string to indicate that the argument was

\$\endgroup\$
  • \$\begingroup\$ It'll raise IndexError: string index out of range, which seems close enough already. It's not generally worth checking for bad args, else every function would have a ton of checks to do. \$\endgroup\$ – Carl Smith May 14 '14 at 2:02
1
\$\begingroup\$

Your code looks pretty good. There are a few improvements that I can see.

Firstly, when following PEP8 (the official Python style convention guide) we want to avoid using multiple statements per line:

# Avoid this style.
if i == len(digits): return bits

# Use this style.
if i == len(digits):
    return bits

Because of your use of multiple statements per line, your code looks like it has an extraneous amount of blank lines (another no-no as defined by PEP8). Fixing the statements will also help mitigate the feeling that your code has too many blank lines.


Secondly, I typically try to avoid declaring functions inside of functions (unless I'm doing something REALLY funky: think async plus decorators plus magic). As many of the answers say here, functions inside functions are useful for creating closures and decorators, both of which are not really needed here in your implementation.


Thirdly, in your for loop within encode() you unpack the tuple returned from zip then take those unpacked elements and put them into a list to extend bits:

# Unpack into `b` and `w`
for b, w in zip(byte(i), byte(i+1)):
    # 'Re-pack' back into list
    bits.extend([b, w])

Instead, you can just take the tuple itself and pass that to extend():

for tup in zip(byte(i), byte(i+1)):
    bits.extend(tup)

Finally, your variable names are a tad ambiguous. They can be guessed (if you know ITF format) but we don't want to necessarily assume others know ITF format or require them to have guess what each of our variables are for.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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