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

Groups > comp.lang.python > #88628

Re: Code critique please

Show key headers only | View raw

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.

Back to comp.lang.python | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

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

csiph-web