[Tutor] please return flys in ointment

[Dave Angel]

On 07/06/2013 05:38 PM, Jim Mooney wrote:
> I reworked my numbers_to_name program so I can actually follow the logic.
> Since I think the best way to learn is to follow something through to
> completion, I'll put it online so I learn how to do that. But naturally,
> lest I look dumb, I'd appreciate criticism, improvements of bad form, or if
> anyone can break it, let me know ;') . Seems to work with a bit of random
> testing, but ya never know. If you did, Msoft wouldn't be putting out those
> updates all the time. Works with Py 27 or 33, but that brings up a
> question: I think most of my programs work with both if I don't do anything
> exotic, except for input(), which is the fly in the ointment. Instead of
> using the routine below, could I redefine input() to use the routine, or it
> that a bad idea?
>
> I have a feeling the leftpad routine could be simplified, but it isn't
> coming to me.
>

General comments:

1) several lines wrapped in the newreader, so I had to paste things 
together.  Presumably this won't happen whereever you put it online, but 
you should make sure and check after uploading it to make sure no such 
problems were introduced.

Also, instead of using the backslash line-continuation character, I 
generally try to use the syntax of the language instead.  So if you have 
a quote which won't fit on one line, but you don't want embedded 
newlines in it either, you can use the implicit concatenation of 
adjacent literals.  Frequently you have to put parens around the whole 
thing to make sure the compiler sees it as one line.  But I think it's 
usually more readable that way.

First example:  the prompt in the input/raw_input lines.


> # Using C:\Python33\python.exe on Win 7 in c:\python33\jimprogs - not
> Windows-specific
> # Also works with python 2.7
>

You always take the data in by input/raw_input.  Why not permit a value 
on the command line?  And even more important, you're mixing 
input/output with algorithm here.

> import sys
>
> # Data
> ones = {'1': 'one', '2': 'two', '3': 'three', '4': 'four', '5': 'five',
> '6': 'six',
> '7': 'seven', '8': 'eight', '9': 'nine'}
>
> tens = {'2': 'twenty', '3': 'thirty', '4': 'forty', '5': 'fifty', '6':
> 'sixty',
> '7': 'seventy', '8': 'eighty', '9': 'ninety'}
>
> doubles = {'0': 'ten', '1': 'eleven', '2': 'twelve', '3': 'thirteen', '4':
> 'fourteen',
> '5': 'fifteen', '6': 'sixteen', '7': 'seventeen', '8': 'eighteen', '9':
> 'nineteen'}
>
> powers_of_1000 = (' thousand', ' million', ' billion', ' trillion',
> ' quadrillion', ' quintillion', ' sextillion', ' septillion', ' octillion',
> ' nonillion', ' decillion')
>
> '''add these later, and also option for dollars-and-cents ending.
> 'vigintillion', 'novemdecillion', 'octodecillion', 'septendecillion',
> 'sexdecillion', 'quindecillion', 'quattuordecillion', 'tredecillion',
> 'duodecillion', 'undecillion',
> 'decillion', 'nonillion'

Those last two you've already implemented

> '''
>
> # Functions
> def make_triplets_from_input():
>      '''Enter a positive integer. A list of triplets in the same order will
> be returned.
>      Raise ValueError to loop on non-integer input, or return 'zero'
> trigger-value of
>      'xxx' if all zeroes are entered. If input is okay, strip leading
> zeroes, then
>      zero-pad leftmost triplet to three characters, so all slices are
> triplets. Adjust
>      input for different Python versions.'''
>      while True:
>          try:
>              if sys.version[0:3] == '2.7':

I'd do the version checking in initialization code, not here.  And I'd 
do it by redefining input to do what it does in 3.3

>                  numbers_str = original = raw_input('Enter a positive
> integer, space \
> separated if desired.')
>              elif sys.version[0:3] == '3.3':
>                  numbers_str = original = input('Enter a positive integer,
> space \
> separated if desired.')

Note:  you explicitly support zero, so this should say  non-negative, 
rather than positive.

I'm not sure why you support spaces between the digits, and not commas, 
but no biggie.  BTW, you also support other white space, like tabs.

>              else:
>                  print('Python versions 2.7 and 3.3 only supported')
>                  sys.exit()
>
>              numbers_str = ''.join(numbers_str.split())
>              if numbers_str == '' or numbers_str == ' ': raise ValueError
>              numbers_int = int(numbers_str)
>              if numbers_int == 0:
>                  print('Zero')
>                  sys.exit()

This is just weird.  You're in the middle of an input routine, and you 
just print something and exit the code??


>              numbers_str = str(numbers_int)
>              if len(numbers_str) > 36: raise ArithmeticError
>              break
>          except KeyboardInterrupt:
>              print('Program cancelled by user')
>              sys.exit()
>          except ValueError as err:
>              print(original, "is not an integer.")
>              continue
>          except ArithmeticError as err:
>              print(original, "is too big.\n999...decillion is max: 10**37-1
> or 36 chars \
> or 12 groups of 3 chars")
>
>      leftpad = len(numbers_str) % 3 # zero-pad left, so we get all
> 3-character triplets
>      if leftpad == 0: leftpad = ''
>      elif leftpad == 2: leftpad = '0'
>      else: leftpad = '00'

Instead of these four lines, how about:
        leftpad = "0" * (-i)%3

>      numbers_str = leftpad + numbers_str
>      triplets = [numbers_str[x:x+3] for x in range(0,len(numbers_str),3)]
>      return (triplets, len(triplets))

Seems a little silly to make this a tuple, when the second value is 
trivially figured from the first.  You could just have the second 
function take a single list argument and take its len() for itself.

And even then, I'd recommend using a slice when calling it instead of 
passing it a number.  Just one more bug lurking in the wings.
>
> def numbers_to_name(triplets, triplen):
>      '''Create a name from each triplet and append the power of ten'''
>      triplen -= 2
>      number_name = ''
>
>      for triplet in triplets:
>          triplet_name = ''
>          first, second, third, last_two = (triplet[0], triplet[1],
> triplet[2], triplet[1:])
>          if triplet == '000':
>              triplen -= 1
>              continue
>          if first > '0':
>              if last_two == '00': # special case - snip extra space separator
>                  triplet_name += ones.get(first) + ' hundred'
>              else:
>                  triplet_name += ones.get(first) + ' hundred '
>          if second == '0':
>              if third > '0':
>                  triplet_name += ones.get(third)
>          elif second == '1':
>              triplet_name += doubles.get(third)
>          elif second > '1':
>              triplet_name += tens.get(second)
>              if third > '0':
>                  triplet_name += '-' + ones.get(third)
>          number_name += triplet_name
>          if triplen > -1:
>              number_name += powers_of_1000[triplen] + ', '
>          triplen -= 1
>      if number_name[-2] == ',':
>          number_name = number_name[0:-2] # special case - snip extraneous
> ending comma
>      return number_name
>
> # Main Program - turn numeric input into number-names
> triplets, triplen = make_triplets_from_input()
> print(numbers_to_name(triplets, triplen))
>

If you included the usual if __name__ ==  test around those two lines, 
you'd be able to use this same file as an importable module.

You should have four functions, not two:

1) parse argv
2) get input from user
3) turn string into list  ( make_triplets_from_input)
4) turn list into string, ready to print  (numbers_to_name)

and then the top-level code does something like:

if __name__ == "__main__":
     if len(argv) == 2:
         raw = parseargv()
     elif len(argv) == 1:
         raw = getinputfromuser()
     else:
            someerrormessage
            exit
     triplets =  make_triplets_from_input(raw)
     print(numbers_to_name(triplets))


-- 
DaveA