Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #88623 > unrolled thread

Code critique please

Back to article view | Back to comp.lang.python

Contents

  Code critique please kai.peters@gmail.com - 2015-04-07 15:43 -0700
    Re: Code critique please Ian Kelly <ian.g.kelly@gmail.com> - 2015-04-07 17:25 -0600
    Re: Code critique please Chris Kaynor <ckaynor@zindagigames.com> - 2015-04-07 16:38 -0700
    Re: Code critique please Cameron Simpson <cs@zip.com.au> - 2015-04-08 09:49 +1000
    Re: Code critique please kai.peters@gmail.com - 2015-04-07 16:51 -0700
    Re: Code critique please Mark Lawrence <breamoreboy@yahoo.co.uk> - 2015-04-08 01:54 +0100
    Re: Code critique please Robert Kern <robert.kern@gmail.com> - 2015-04-08 13:25 +0100
    Re: Code critique please Peter Otten <__peter__@web.de> - 2015-04-09 12:26 +0200

#88623 — Code critique please

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, 
    c = 0                                # each representing one bit
    for bit in blist:                   
        if bit:                
            draw.point((x + c, y), fcolor)
            
        c += 1
    return

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

epdfilename   = 'c:\\temp\\drawtest2.epd'
imagefilename = 'c:\\temp\\drawtest2.png'

EPD_2_Image(epdfilename, imagefilename)

[toc] | [next] | [standalone]

#88628

On Tue, Apr 7, 2015 at 4:43 PM,  <kai.peters@gmail.com> wrote:
> def RenderByte(draw, byte, x, y):

Python function and method names customarily use the
lowercase_with_underscores style.

>     blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8 elements,

There's no guarantee that the resulting list will have 8 elements
here. The result of bin() will use the minimum number of digits needed
to represent the value, which has nothing to do with the size of a
byte. If you want exactly 8, use str.zfill to add zeroes.

Also, lstrip('0b') will remove *all* occurrences of the characters '0'
and 'b' from the left of the string, not just the exact substring
'0b'. So for example, bin(0).lstrip('0b') would return the empty
string since all the characters in the string are either '0' or 'b'.

That said, all this string manipulation to convert from an int to a
list of bits smells a bit funky to me. I'd probably write a generator
to yield the bits from the number. Something like:

    def bits(number, length=None):
        if length is None:
            length = number.bit_length()
        for i in range(length):
            yield (number >> i) & 1

Now you can iterate directly over bits(byte, 8), or if you really want
a list then you can call list(bits(byte, 8)).

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

Instead of incrementing c on each loop, use enumerate.

    for c, bit in enumerate(blist):

>     return

Not needed if you're not returning a value. It's perfectly okay to
just let execution "fall off" the end of the function.

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

Recommend using print's function syntax for forward compatibility with Python 3:

    print('file not found: ' + edpfilename)

Also consider doing "from __future__ import print_function" at the top
of the file so that print really is a function, although it doesn't
matter in this case.

> xdim   = 1024
> ydim   = 1280
> header = 16
> black  = 0
> white  = 1
> bcolor = black
> fcolor = white

Constants, particularly global ones, are conventionally
UPPERCASE_WITH_UNDERSCORES.

> EPD_2_Image(epdfilename, imagefilename)

If you use the construct

    if __name__ == '__main__':
        EPD_2_Image(epdfilename, imagefilename)

then the function will automatically be called when the file is
executed as a script, but it will also let you import the file from
other modules and call it on your own terms, should you ever desire to
do that.

[toc] | [prev] | [next] | [standalone]

#88630

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

[toc] | [prev] | [next] | [standalone]

#88631

On 07Apr2015 15:43, kai.peters@gmail.com <kai.peters@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@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@aol.com>

[toc] | [prev] | [next] | [standalone]

#88632

On Tuesday, 7 April 2015 15:43:44 UTC-7, kai.p...@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, 
>     c = 0                                # each representing one bit
>     for bit in blist:                   
>         if bit:                
>             draw.point((x + c, y), fcolor)
>             
>         c += 1
>     return
> 
> # -----------------------------------------------------------------------------
> 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
>     
>     # 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
> # -----------------------------------------------------------------------------
>     
> xdim   = 1024
> ydim   = 1280
> header = 16
> black  = 0
> white  = 1
> bcolor = black
> fcolor = white
> 
> epdfilename   = 'c:\\temp\\drawtest2.epd'
> imagefilename = 'c:\\temp\\drawtest2.png'
> 
> EPD_2_Image(epdfilename, imagefilename)


Thanks for taking the time to give such detailed suggestions - very much appreciated!

K

[toc] | [prev] | [next] | [standalone]

#88640

On 07/04/2015 23:43, kai.peters@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.
>
> import os, sys
> from PIL import Image, ImageFont, ImageDraw
>

As you've had plenty of answers I'll just say that PIL is pretty old 
now.  Are you aware of the fork called Pillow? 
https://pillow.readthedocs.org/

-- 
My fellow Pythonistas, ask not what our language can do for you, ask
what you can do for our language.

Mark Lawrence

[toc] | [prev] | [next] | [standalone]

#88661

On 2015-04-08 01:54, Mark Lawrence wrote:
> On 07/04/2015 23:43, kai.peters@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.
>>
>> import os, sys
>> from PIL import Image, ImageFont, ImageDraw
>>
>
> As you've had plenty of answers I'll just say that PIL is pretty old now.  Are
> you aware of the fork called Pillow? https://pillow.readthedocs.org/

Pillow uses the name "PIL" for its package name too, in the interest of being 
drop-in compatible.

https://pillow.readthedocs.org/porting-pil-to-pillow.html

-- 
Robert Kern

"I have come to believe that the whole world is an enigma, a harmless enigma
  that is made terrible by our own mad attempt to interpret it as though it had
  an underlying truth."
   -- Umberto Eco

[toc] | [prev] | [next] | [standalone]

#88705

kai.peters@gmail.com wrote:

> I just wrote this bit (coming from Pascal) 

>     if (((xdim / 8) * ydim) + header) <> filesize:

Yeah, either Pascal or Barry Warsaw is your uncle ;)

https://www.python.org/dev/peps/pep-0401/

> and am wondering how seasoned
> Python programmers would have done the same? Anything terribly non-python?

> def RenderByte(draw, byte, x, y):

Please read PEP 8 for naming conventions etc.

The function at the core of your code

> def RenderByte(draw, byte, x, y):
> 
>     blist = list(bin(byte).lstrip('0b')) # turn byte into list with 8
>     elements,
>     c = 0                                # each representing one bit
>     for bit in blist:
>         if bit:
>             draw.point((x + c, y), fcolor)
>             
>         c += 1
>     return

can be fixed/micro-optimised, and if I were to do that I'd precreate a 
lookup table that maps every byte (i. e. value in the range 0...255) to a 
sequence of offsets 

lookup = [
    (),      # for b"\x00" the inner loop is empty
    (0),
    (1),
    (0, 1),
    ...
    (0, 1, 2, 3, 4, 5, 6, 7), # for b"\xFF" the inner loop comprises 
                              # all 8 bits
]
or to a 8x1 image, but my idea of a "seasoned Python programmer" will try to 
keep the big picture in mind -- and that is that handling individual 
bits/pixels in Python is inefficient. Therefore many libraries tend to offer 
an alternative that is both easier to use and faster.

In this case that's

   img = Image.frombytes("1", (width, height), data)
 
and with the extra comfort of reading the size from the EPD file

from PIL import Image
import struct

EPDFILE = "tmp.epd"
PNGFILE = "tmp.png"

class EPDError(Exception):
    pass

with open(EPDFILE, "rb") as f:
    header = f.read(16)
    width, height, colordepth = struct.unpack("<HHb", header[1:6])
    if colordepth != 1:
        raise EPDError("Unsupported color depth {}".format(colordepth))
    data = f.read()
    datasize = width // 8 * height
    if len(data) != datasize:
        raise EPDError(
            "Inconsistent pixel data size. "
            "Expected {} but got {} bytes".format(
                datasize, len(data)))
    img = Image.frombytes("1", (width, height), data)
    img.save(PNGFILE)


I'm assuming the header follows the format spec given in this PDF:

http://www.pervasivedisplays.com/_literature_112691/Developer_Guide_for_Demo_Kit

PS: I thought that Image.frombytes() was already pointed out to you, but I 
failed to find it in the archives. So there.

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web