5
\$\begingroup\$

I've read this question about computing the digital root of some integer, and wondered how it would look like in Python.

As a reminder,

"If you take the digits of any number and add them together, and then add the digits of the resulting number together, and continue doing that until you get a single digit, that single digit is the digital root of the original number."

This is not much about the algorithm performance itself, since it was shown in this answer that instead of summing the digits, you could do a simple division by 9 to get the same result. The question is more about the readability, elegance, and pythonicity (if this word exists) of my code. I checked its correctness with a tool from Thonky.

More, I'd be interested in knowing your opinion on the way I deal with sys.argv, the user interaction, and the benchmarking.

Finally, is it OK to use the int type for such long numbers? It seems to work fine even with 1000-digit numbers, but intuitively it shouldn't...

Any suggestion would be highly appreciated. Thanks.


digitalroot.py

# -*- encoding: utf-8 -*-
import time
import random
import sys
def request_user_number():
 while True:
 try:
 nb = int(raw_input('Enter a number\n'))
 return nb
 except (ValueError, NameError):
 print 'This is not a number. Try again.'
def request_user_again():
 while True:
 try:
 ans = str(raw_input('New computation? (y/n)\n')).lower()
 if ans[0] == 'n':
 again = False
 print 'Bye !'
 break
 elif ans[0] == 'y':
 again = True
 break
 else:
 print 'Did not understand. Try again.'
 except IndexError:
 print 'Say something.'
 return again
def digits_generator(nb):
 tmp = nb
 while tmp != 0:
 yield tmp % 10
 tmp /= 10
def digital_root(nb):
 if nb < 10:
 return nb
 else:
 s = 0
 for d in digits_generator(nb):
 s += d
 return digital_root(s)
def usage():
 print 'Wrong usage'
 print '\'python digitalroot.py ui\' for a user chosen number'
 print '\'python digitalroot.py benchmark nb_size tries\' for benchmarking'
 exit(1)
if __name__ == '__main__':
 if len(sys.argv) < 2 or sys.argv[1] not in ['ui', 'benchmark']:
 usage()
 if sys.argv[1] == 'ui':
 again = True
 while again:
 nb = request_user_number()
 print 'Computing digital root of', nb
 start_time = time.clock()
 dr = digital_root(nb)
 end_time = time.clock()
 print nb, '-->', dr, '(time :', end_time-start_time, 's)'
 again = request_user_again()
 elif sys.argv[1] == 'benchmark':
 try:
 nb_size = int(sys.argv[2])
 tries = int(sys.argv[3])
 except (ValueError, NameError, IndexError):
 usage()
 global_start = time.clock()
 s = 0
 for i in xrange(tries):
 nb = random.randint(10**(nb_size-1), 10**nb_size-1)
 start_time = time.clock()
 d = digital_root(nb)
 end_time = time.clock()
 s += end_time-start_time
 if random.randint(1, tries/5) == 1:
 print 'Random control : i =', i, ',', nb, '-->', d
 print nb_size, 'digits,', tries, 'tries'
 print s, 'seconds (average =', float(s)/tries, 's/try)'
 print 'Total benchmark time :', time.clock()-global_start
asked Jul 21, 2016 at 11:45
\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

Here are a few things:

Instead of

if ans[0] == 'n':
 again = False
 print 'Bye !'
 break
elif ans[0] == 'y':
 again = True
 break
...
return again

You could just return right away:

if ans[0] == 'n':
 print 'Bye !'
 return False
elif ans[0] == 'y':
 return True

When having a constant list, use a tuple (it makes it immediately obvious that it will not be modified, because it can't (even though it can be overridden, of course):

if len(sys.argv) < 2 or sys.argv[1] not in ['ui', 'benchmark']
if len(sys.argv) < 2 or sys.argv[1] not in ('ui', 'benchmark')

Use more functions

Currently all your code executing the things resides under if __name__ == "__main__". But since it is quite longish, I would at least put it into a main() function and only call that. But, even better, would be to put the two possible options for the problem into functions. This also allows us to define a decorator timeit to outsource the timing:

import time
def timeit(func):
 def wrapper(*arg, **kw):
 t1 = time.time()
 res = func(*arg, **kw)
 t2 = time.time()
 return (t2 - t1), res
 return wrapper
@timeit
def digital_root(nb):
 ...
def ui():
 again = True
 while again:
 nb = request_user_number()
 print 'Computing digital root of', nb
 time, dr = digital_root(nb)
 print '{} --> {} time: {}s'.format(nb, dr, time)
 again = request_user_again()
def benchmark(nb_size, tries):
 benchmark_start = time.clock()
 s = 0
 for i in xrange(tries):
 nb = random.randint(10**(nb_size-1), 10**nb_size-1)
 t, d = digital_root(nb)
 s += t
 if random.randint(1, tries/5) == 1:
 print 'Random control : i =', i, ',', nb, '-->', d
 print nb_size, 'digits,', tries, 'tries'
 print s, 'seconds (average =', float(s)/tries, 's/try)'
 print 'Total benchmark time :', time.clock()-benchmark_start
def main():
 if sys.argv[1] not in ('ui', 'benchmark'):
 usage()
 if sys.argv[1] == 'ui':
 ui()
 else:
 if len(sys.arg) != 4:
 usage()
 try:
 opts = (int(x) for x in sys.argv[2:])
 except ValueError:
 usage()
 benchmark(*opts)
if __name__ == "__main__":
 main()

We could make the main function even simpler if the two functions took the same number of arguments, allowing us to do the argument checking once and then use a dictionary for the functions. Since they are not it becomes slightly more complecated because we also need to store how many arguments we need:

def main():
 mode, opts = sys.argv[1], sys.argv[2:]
 # mode, *opts = sys.argv[1:] # python 3.x
 func = {"ui": (ui, 0), "benchmark": (benchmark, 2)}
 if mode in func and len(opts) == func[mode][1]:
 try:
 opts = (int(opt) for opt in opts)
 except ValueError:
 usage()
 func[mode][0](*opts)
 else:
 usage()

Alternatively, it is easier to ask forgiveness than permission:

def main():
 mode, opts = sys.argv[1], sys.argv[2:]
 # mode, *opts = sys.argv[1:] # python 3.x
 func = {"ui": ui, "benchmark": benchmark}
 try:
 opts = (int(opt) for opt in opts)
 func[mode](*opts)
 except (KeyError, TypeError, ValueError): # KeyError for wrong func name, TypeError for nopts mismatch, ValueError if opts not numbers
 usage()

Where it is arguably whether the except might not match too many different failure scenarios, so it might be better to go with one of the more explicit approaches above.

answered Jul 21, 2016 at 13:26
\$\endgroup\$
2
  • \$\begingroup\$ This gives me many more paths to explore, thank you ! I find the decorator thing particularly nice :) \$\endgroup\$ Commented Jul 21, 2016 at 13:55
  • \$\begingroup\$ Yes, decorators seem a bit arcane at first, but are really usefull to avoid code duplication or even code modification on-the-fly, without changing any already defined function (You can also use them e.g. like digitalroots = timeit(digitalroots), instead of @timeit; def digitalroots: ...) \$\endgroup\$ Commented Jul 21, 2016 at 14:00
7
\$\begingroup\$

I think your algorithmic part is exceedingly verbose. One of the nicenesses of Python is how it can lead to code that reads almost like English. And for this particular problem, even leaving performance considerations aside, I think iteration is clearer than recursion. So I would rewrite things as:

def digits_of(number):
 """Yields the digits of an integer."""
 while number > 0:
 yield number % 10
 number //= 10
def digital_root(number):
 """Computes the digital root of an integer."""
 while number > 9:
 number = sum(digits_of(number))
 return number

I'm pretty sure my 6 year old daughter could make sense of that last function with very little help.

answered Jul 21, 2016 at 13:52
\$\endgroup\$
2
  • \$\begingroup\$ You've got a point. I love recursion. But your code is way more clear. \$\endgroup\$ Commented Jul 21, 2016 at 13:55
  • 1
    \$\begingroup\$ Renaming digital_root to sum_of_digits_of may get you comparable clarity using recursion. \$\endgroup\$ Commented Jul 21, 2016 at 14:04
5
\$\begingroup\$

Regarding digits_generator

You don't need to use a new variable. You could use nb directly.

You may use the fact that non-0 integers are considered as True in a boolean context and 0 is considered as False to write if nb:.

Anytime you use % and / on the same values, you could use divmod. Here you could write:

def digits_generator2(nb):
 while nb:
 q, r = divmod(nb, 10)
 yield r
 nb = q

or:

def digits_generator2(nb):
 while nb:
 nb, r = divmod(nb, 10)
 yield r

You could avoid having the magic number 10 in your code by using a parameter with a meaningful name and a useful default value like:

def digits_generator2(nb, base=10):
 while nb:
 nb, r = divmod(nb, base)
 yield r
answered Jul 21, 2016 at 16:27
\$\endgroup\$
0

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.