[Tutor] Pythonify this code!

[Dave Angel]

Muhammad Ali wrote:
> Hi,
>
> This is my first attempt at python. I tried my hand at steganography for
> fun. The code includes separate, combine, encode, decode functions. The
> separate function takes a number and it's base and returns a list containing
> each digit of the number in a list, vice versa for combine. The encode
> function encodes each char's ascii value into each pixel's RGB values at the
> least significant end. The decode function does the opposite. e.g.,
>
> a = 97, so if we have at a pixel r, g, b (230, 107, 155) it will become
> (230, 109, 157) when it's encoded.
>
> Since I am new and don't speak the python idiom yet, I would like the
> experts to pythonify the following code as much as possible for both python
> 2.6 and 3.0. Any other suggestion to improve the quality would also be
> highly appreciated
>
> Thanks a lot!
>
>
> #The code starts here:
>
> def separate(num, base):
>     li = []
>     while num / base > 0:
>         li.insert(0, num % base)
>         num = num / base
>
>     li.insert(0,num)
>     return li
>
> def combine(tup, base):
>     num = 0
>     mul = pow(base, len(tup) - 1)
>     for i in tup:
>         num = num + i * mul
>         mul = mul / base
>
>     return num
>
>
> #Will make changes to the original image and not to a copy! You have been
> warned!
> import Image
>
> def encode(img, text):
>     x = 0
>     y = 0
>     height, width = img.size
>
>     text = text + "~~"
>
>     if len(text) > height * width:
>         return false
>
>     pix = img.load()
>
>     for c in text:
>         li = separate(ord(c), 10)
>
>         if(len(li) == 1):
>             li.insert(0, 0)
>             li.insert(0, 0)
>         elif(len(li) == 2):
>             li.insert(0, 0)
>
>         r, g, b = pix[x, y]
>
>         r = r - (r % 10) + li[0]
>         if (r > 255):
>             r = r - 10
>
>         g = g - (g % 10) + li[1]
>         if (g > 255):
>             g = g - 10
>
>         b = b - (b % 10) + li[2]
>         if (b > 255):
>             b = b - 10
>
>         pix[x,y] = (r,g,b)
>
>         if y == width - 1:
>             y = 0
>             x = x + 1
>         else:
>             y = y + 1
>
>     img.save(img.filename)
>
> def decode(img):
>     x = 0
>     y = 0
>     text = ""
>     c = ""
>     height, width = img.size
>     pix = img.load()
>     while 1:
>         r, g, b = pix[x, y]
>         if (c == '~') and chr(combine([r % 10, g % 10, b % 10], 10)) == '~':
>             return text[:len(text) - 1]
>
>         c = chr(combineUnits([r % 10, g % 10, b % 10], 10))
>         text = text + c
>
>         if y == width - 1:
>             y = 0
>             x = x + 1
>         else:
>             y = y + 1
>
>   
Pretty good for your first Python code.  Clearly you have some 
experience in other language(s).  Comments follow, no particular order.

Import should be at begin of module, unless it's just plain not 
possible.  Functions should have doc-strings, explaining what they do, 
and what the arguments look like.  For example, first two functions 
wouldn't work with floats, only ints.  There also should be comments 
stating what ranges of values are legal.  For example, num is a positive 
integer, and base is an integer between one and 250 or so.

separate() could be simplified and optimized, some depending on the 
range of expected parameters, but below we'll see that it should be 
replaced entirely.  So don't optimize prematurely.

separate() and combine() both naturally want to do the digits in the 
oppposite order, and nothing else cares.  But again, it's premature 
optimization.

Looking at encode(), we see that the only call to separate() is with 10 
as a base.  And it immediately takes the nicely formatted number, and 
pads it to 3 digits.  Sounds like a standard formatting operation to me, 
so why not just use the format method with a 3 for width?  Then loop 
over the string, or even use a list comprehension on it.

encode() refers to }false", but the built-in value is called "False"

encode() uses repetitive code to combine two tuple/lists.  Ideal for a 
separate function, perhaps using zip().  Then we could replace 14 lines 
with:

        pix[x, y] =pixel(pix[x, y], li)

in encode(), row/column incrementing could be simplified, and common 
case sped up:

        y += 1
        if y == width:  y, x = 0, x+1

Incidentally, aren't you processing the pixels in column-major order?  
Doesn't matter much for tiny images, but for symmetric designs, it's 
usually better to keep memory accesses localized, to reduce swapping.  I 
could be all wet here, though, just going on memory.

decode() refers to combineUnits() in one of its two calls, but it's 
actually called combine().  Further, there's no need to call it twice.  
Better would be to have two "c" variables, maybe calling one of them 'lastc'

        lastc = c
        c = chr(combineUnits([r % 10, g % 10, b % 10], 10))
        if (c == '~') and lstc == '~':
            return text[:len(text) - 1]

In decode(),  while 1    is usually spelled:    while True

Hope these help.
DaveA