current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I was asked to provide some example code for a job interview and didn't have anything to offer, so I wrote the following function.

A barcode generator may seem a bit basic, but there's some logic behind the choice:

  • It's something I recently needed, so it's not entirely contrived.
  • It's simple enough to do in one function, so it can be reviewed easily.
  • It's complex enough that a new programmer would make a mess of it.
  • It's cute. You can render the generated barcode in a browser.
  • It's useful, so I could make a little gist out of it.

The barcode is in the Interleaved 2 of 5 Format. It's a simple format, and there's a Wikipedia article that explains it well.

Update: A new version of this code has been posted here. The new code includes many of the suggestions made below.

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.

        import barcode
        svg = barcode.render('0123456789')

    Strings are used to avoid problems with insignificant zeros, and allow
    for iterating over the bits in a byte [for zipping].

    # Wikipedia, ITF Format: http://en.wikipedia.org/wiki/Interleaved_2_of_5
    '''

    # Encode: Convert the digits into an interleaved string of bits, with a
    # start code of four narrow bars [four zeros].

    bits = '0000'
    bytes = {
        '0': '00110', '5': '10100',
        '1': '10001', '6': '01100',
        '2': '01001', '7': '00011',
        '3': '11000', '8': '10010',
        '4': '00101', '9': '01010'
        }

    for i in range(0, len(digits), 2):

        # get the next two digits and convert them to bytes
        black, white = bytes[digits[i]], bytes[digits[i+1]]

        # zip and concat to `bits` 5 pairs of bits from the 2 bytes
        for black, white in zip(black, white): bits += black + white

    # Render: Convert the string of bits into an SVG string and return it.

    svg = '<svg height="50">'
    bar = '<rect x="{0}" y="0" width="{1}" height="50" {2}></rect>'
    pos = 5 # the `x` attribute of the next bar [position from the left]

    for i, bit in enumerate(bits):

        width = int(bit) * 2 + 2
        color = '0,0,0' if (i % 2 == 0) else '255,255,255'
        style = 'style="fill:rgb({0})"'.format(color)

        svg += bar.format(pos, width, style)
        pos += width

    return svg + '</svg>'
share|improve this question
1  
I've removed the updated code from the question. Reviewing an additional code revision is not encouraged as it could make the review process (within the question itself) confusing. You may instead post the new code as a new question for further review. –  Jamal♦ May 12 at 16:53
add comment

3 Answers

up vote 3 down vote accepted

The code generally looks pretty good, but I think there are some things you could fix up here.

Don't forget the stop code

You have the start code of 0000 but you've forgotten the stop code which is 100. When you calculate, you could just tack it onto the end with bits += '100'

Use SVG <def>s

Since the purpose is to create an SVG barcode, it make sense to tighten the resulting SVG. You may already know how to use a <def> in SVG and it definitely makes things a little easier to understand here. The way your code is currently structures, it creates four different styles of <rect> which are all combinations of narrow/wide and black/white. You could predefine each of those shapes and then simply intantiate them within the body of the svg code. That would make the definition for svg and bar like this:

svg = '''<svg height="50"><defs>
<g id="b0"><rect x="0" y="0" width="2" height="50"/></g>
<g id="b1"><rect x="0" y="0" width="4" height="50"/></g>
</defs>'''
bar = '<use xlink:href="#b{0}" x="{1}" y="0" {2}/>'

Using it would then be 

svg += bar.format(bit, pos, style)

An improvement would be to create both bars and spaces like this:

<g id="b0"><rect x="0" y="0" width="2" height="50" style="fill:rgb(0,0,0)"/></g>
<g id="b1"><rect x="0" y="0" width="4" height="50" style="fill:rgb(0,0,0)"/></g>
<g id="s0"><rect x="0" y="0" width="2" height="50" style="fill:rgb(255,255,255)"/></g>
<g id="s1"><rect x="0" y="0" width="4" height="50" style="fill:rgb(255,255,255)"/></g>

Then your loop could look then like this:

for i, bit in enumerate(bits):
    width = int(bit) * 2 + 2
    svg += bar.format('bs'[i%2], bit, pos)
    pos += width

Although that may look verbose, it actually is over 2k shorter for a 12 digit barcode.

Think carefully about data representation

Your code currently translates digits into a series of '1' and '0' characters and then translates again into SVG rectangles. Why not eliminate a step? Your code could just as easily translate them in a single operation.

Use list comprehensions instead of for loops

The use of list comprehensions is almost always faster than a for loop in Python, so we use them when we can. It also tends to make the code shorter. So for example, we could change your loop to calculate the string of bar code bits to calculate all the black bits and then all the white bits like this:

black = "".join([bytes[i] for i in digits[0::2]])
white = "".join([bytes[i] for i in digits[1::2]])
# shuffle them together
databits = "".join("".join(i) for i in zip(black,white))
# create the full bar code string with start and stop
bits = "".join(['0000',databits,'100'])

Doing this with join also saves time. Appending strings with += is very slow in Python.

Prefer xrange to range

When you use range, a whole list object is created, using memory. With xrange, a generator is created instead populated which can save memory. For any reasonably sized bar code this won't make much difference here, but it's good practice for Python 2.7. In Python 3, xrange doesn't exist and range creates a generator, so keep that in mind if you change versions.

Don't draw more than you have to

You're drawing one <rect> for every bar or space, but it's really not necessary. Instead, you could create one larger rectangle that's the "background" space color and then only draw the black bars.

share|improve this answer
    
Thank you so much. The stop code: nice catch. I'd push back a little on the second two points, but will think on it some more. The last two points are very good. I was aiming for Python 2 and 3, so a conditional range=xrange would help. Thanks again. I'll take it all on board. Nice answer. –  Carl Smith May 10 at 22:12
1  
@PongWizard: The use of the SVG <defs> looks a little messy the way I've described with all static code, but it could also be automatically constructed, which might be nice if you decided to output some other format. Also, consider that if you changed the representation from {0,1} to {b,B,s,S} (for small bar, wide bar, small space, wide space), you could emit the SVG in a single simple list comprehension. –  Edward May 10 at 23:46
    
I took your advice on board and rewrote the whole thing. I sacrificed some simplicity to make things more efficient, but I think it has a nice balance now. I updated the gist too. Thanks again. –  Carl Smith May 12 at 14:37
add comment
up vote 1 down vote

There's one point which Edward's answer doesn't mention, although his changes might eliminate it.

        for black, white in zip(black, white):

The limit to the number of variable names you can use is large enough that you don't have to reuse them like this. Using different names for what are effectively different variables aids comprehension.

share|improve this answer
    
It wasn't an attempt to reduce the number of names. The function repeatedly breaks things into pairs of black and white. First it's pairs of digits, then pairs of bits from the encodings, then pairs of widths for the bars, so it seemed to be clearer to just have black and white and just keep 'processing' them as pairs, then to have for black_bit, white_bit in zip(black_byte, white_byte). Your suggestion isn't wrong, but doing it your way is equally awkward. –  Carl Smith May 13 at 21:20
1  
@PongWizard, I'm not commenting on awkwardness but on confusion. It's not ideal to have one name mean different things at different points in a method; but to have it meaning different things at different points in a statement is a sure-fire way to confuse the maintenance programmer who's still picking up the language. (As a follow-up, even and odd might be more transparent names than black and white; and for even_bit, odd_bit in zip(even_byte, odd_byte) helps communicate the effect and purpose of the zip). –  Peter Taylor May 13 at 22:17
    
Thanks for your patience Peter. You made the point very well in that comment. You're right; that is clearer. I'll update the gist soon. Thanks again. –  Carl Smith May 13 at 22:23
add comment
up vote 1 down vote

dont overwrite builtins. bytes is a builtin name.

keep pep8 and readability in mind - e.g, dont do inline for loops.

use more builtins, e.g, use zip to get 2 items at a time instead of playing with indexes:

from itertools import chain
bits = '0000'
for black, white in zip(digits, digits[1:]):
   bits += ''.join(chain(*zip(bytes[black], bytes[white])))
share|improve this answer
    
I did think about changing bytes. You're right; will do. –  Carl Smith May 13 at 21:12
    
Personally I can't stand PEP8. If you're working with others, and PEP8 is what people agree on, then fine, but it's a terrible style. Your point still stands though. –  Carl Smith May 13 at 21:15
2  
shudder PEP8 is top notch. –  rikAtee May 13 at 21:32
add comment

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.