Code critique please

[Chris Kaynor]

On Tue, Apr 7, 2015 at 3:43 PM,  <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,

A couple of comments on this line: lstrip strips based on characters,
and even without that, the 8 elements comment is not correct. bin()
only returns enough to hold the value without leading zeros, and the
lstrip will remove any leading zeros, even if it did produce them.

Additionally, if byte is outside the [0, 255] range, bin could produce
more than 8 elements. Your loop will work in any case, but it may
cause incorrect behavior, and in any case, the comment is wrong.

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

Rather than keeping a manual counter, this could be better written
using enumerate:

for c, bit in enumerate(blist):
    if bit:
        draw.point((x + c, y), fcolor)

>     return

This return is unneeded. If a function falls off the end, it
implicitly has a bare "return" at its end. Note that a bare return (or
falling off the end) will cause the function to return None (all
functions have a return value).

>
> # -----------------------------------------------------------------------------
> 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
>
>     # is this a valid EPD file?
>     filesize = os.path.getsize(epdfilename)
>     if (((xdim / 8) * ydim) + header) <> filesize:
>         print 'incorrect file size: ' + edpfilename
>         return

Both of these would probably be better as exceptions, rather than
prints. When running the function via a batch script, you can always
catch the exception and do something with it, while with a print, the
caller will have a hard time determining whether the function worked
or not. This could be important if you ever try to wrap this function
into a GUI (you may want a dialog rather than a command prompt
textural error, on stdout no less), or call the function with
batching, where you may want to report the failures via other means
(collect them all and include them in an e-mail report, for example).

For example, "raise OSError('file not found' + edpfilename)". Also, in
Python, the perfered dogma is "better to ask forgiveness than
permission" (aka, try the operation and let it fail if invalid),
rather than "look before you leap" (validate correct inputs, and fail
early). Basically, in this case, you could just remove the first check
(the second is probably worthwhile), and let the getsize fail if the
file does not exist.

Raising the exceptions will play better with batching (its easier to
detect errors and report them en-mass after processing). Avoiding LBYL
is generally useful for files in case other processes mess with them
after your check.

>
>     # blow old destination file away
>     if os.path.isfile(imagefilename):
>         print 'deleting old dest. file: ' + imagefilename
>         os.remove(imagefilename)

This is probably unneeded, and could cause issues with deleting the
file then failing out for various reasons. I generally prefer to leave
files intact until the last possible second. Thi sis nice if the code
fails for some reason.

>
>     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
> # -----------------------------------------------------------------------------
>
> xdim   = 1024
> ydim   = 1280
> header = 16
> black  = 0
> white  = 1
> bcolor = black
> fcolor = white

By convention, constants in Python are denoted by using all capital
letters with under scores.

>
> epdfilename   = 'c:\\temp\\drawtest2.epd'
> imagefilename = 'c:\\temp\\drawtest2.png'
>
> EPD_2_Image(epdfilename, imagefilename)
> --
> https://mail.python.org/mailman/listinfo/python-list