Code critique please

[Cameron Simpson]

On 07Apr2015 15:43, kai.peters at gmail.com <kai.peters at gmail.com> wrote:
>I just wrote this bit (coming from Pascal) and am wondering how seasoned Python programmers would have done the same? Anything terribly non-python?
>
>As always, thanks for all input.
>K
>
>"""
> Creates a PNG image from EPD file
>"""
>
>import os, sys
>from PIL import Image, ImageFont, ImageDraw
>
># -----------------------------------------------------------------------------
>def RenderByte(draw, byte, x, y):
>    blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8 elements,

Remark: .lstrip does not do what you think.
You want:

    blist = list(bin(byte)[2:]) # turn byte into list with 8 elements,

to skip over the leading "0b". Otherwise, with lstrip, consider what would 
happen with a low value byte, with multiple leading zeroes. (Actually, on 
reflection, you might get away with it - but probably not, and anyway would be 
fragile against changing the ordering of the bits.)

I'd be popping off the least or most siginificant bit myself with "&" and "<<" 
or ">>". It might be wordier, bit it would be more in keeping with the actual 
bitwise operations going on.

>    c = 0                                # each representing one bit
>    for bit in blist:
>        if bit:
>            draw.point((x + c, y), fcolor)
>        c += 1

This might be more Pythonic written:

  for c, bit in enumerate(blist):

which will emit (0, b0), (1, b1) and so forth for bit 0, bit 1 etc (where bit 0 
is the leftmost from your list, etc). Avoids the "c = 0" and the "c += 1" and 
also makes your loop robust against adding control flows later which might skip 
the final "c += 1" at the end, even by accident.

>    return

You generally do not need a trailing return with no value; it is implicit.

>def EPD_2_Image(edpfilename, imagefilename):
>    # get out of here if EPD file not present
>    if not os.path.isfile(epdfilename):
>        print 'file not found: ' + edpfilename
>        return

Traditionally one would raise an exception instead of returning. Eg:

  if not os.path.isfile(epdfilename):
    raise ValueError("missing file: %r" % (epdfilename,))

and have the caller handle the issue. Similarly for all the other pre-checks 
below.

Also, it is normal for error messages to be directed to sys.stderr (this allows 
them to be logged independently of the program's normal output; for example the 
normal output might be sent to a file, but the error messages would continue to 
be displayed on screen). So were you to iussue a print statement (instead of an 
exception) you would write:

  print >>sys.stderr, ....

or in Python 3:

  print(...., file=sys.stderr)

>    # is this a valid EPD file?
>    filesize = os.path.getsize(epdfilename)
>    if (((xdim / 8) * ydim) + header) <> filesize:
>        print 'incorrect file size: ' + edpfilename
>        return
>
>    # blow old destination file away
>    if os.path.isfile(imagefilename):
>        print 'deleting old dest. file: ' + imagefilename
>        os.remove(imagefilename)
>
>    print 'processing...'
>
>    # set up PIL objects
>    img  = Image.new('1', (xdim, ydim), bcolor)   # '1' = Bi-tonal image
>    draw = ImageDraw.Draw(img)
>
>    # read entire EPD file into byte array (without the header)
>    content = bytearray(open(epdfilename, 'rb').read())[16:]
>
>    # image coord origin at top/left according to PIL documentation
>    pos = 0
>    for y in range(ydim):
>        x = 0
>        for byte in range(xdim / 8):   # 8 pixels 'stuffed' into one byte
>            RenderByte(draw, content[pos], x, y)
>            pos += 1
>            x   += 8
>
>    img.save(imagefilename)   # format is inferred from given extension
>    print 'done.'
>
>    return

Again, this "return" can be implicit.

># -----------------------------------------------------------------------------
>
>xdim   = 1024
>ydim   = 1280
>header = 16
>black  = 0
>white  = 1
>bcolor = black
>fcolor = white
>
>epdfilename   = 'c:\\temp\\drawtest2.epd'
>imagefilename = 'c:\\temp\\drawtest2.png'

Normally these values would be at the top of your program, and named in 
UPPER_CASE to indicate that they are like "constants" in other languages. So 
you might put this:

  XDIM   = 1024
  YDIM   = 1280
  HEADER_SIZE = 16
  BLACK  = 0
  WHITE  = 1
  BCOLOR = BLACK
  FCOLOR = WHITE

  EPD_FILENAME   = 'c:\\temp\\drawtest2.epd'
  IMAGE_FILENAME = 'c:\\temp\\drawtest2.png'

at the top of the script.

I notice you have a bare [16:] in your "content =" line. Should that not say 
"[HEADER_SIZE:]" ?

>EPD_2_Image(epdfilename, imagefilename)

I tend to write things like this as thought they could become python modules 
for reuse. (Often they do; why write something twice?)

So the base of the script becomes like this:

if __name__ == '__main__':
  EPD_2_Image(EPD_FILENAME, IMAGE_FILENAME)

In this way, when you invoke the .py file directly __name__ is "__main__" and 
your function gets run. But it you move this all into a module which may be 
imported, __name__ will be the module name, an so not invoke the main function.  
The importing code can then do so as it sees fit.

Cheers,
Cameron Simpson <cs at zip.com.au>

I heard a funny one this weekend.  I was belaying a friend on a very short
problem and when she was pumped out she told me to "Let me down" and my
other friend that was standing nearby said.  "You were never UP!".
        - Bryan Laws <bryanlaws at aol.com>