9
\$\begingroup\$

I'm new to Python and as a first exercise, I've written a function (module?) to calculate Kaprekar's constant using Python 3.

I'd like some feedback on whether this is Pythonic enough, and if I can improve it.

import collections

def kaprekar(value):   
    print("Starting value: %d" % value)           

    # Check our range
    if value < 1 or value > 9998:
        print("Input value must be between 1 and 9998, inclusive.")
        return

    numstr = str(value)

    # Pad with leading 0s if necessary
    numstr = '0' * (4 - len(numstr)) + numstr    

    # Make sure there are at least two different digits
    if len(collections.Counter(numstr)) == 1:
        print("Input value must consist of at least two different digits.")
        return

    # If we've gotten this far it means the input value is valid

    # Start iterating until we reach our magic value of 6174
    n = 0
    while (value != 6174):
        n += 1

        numstr = str(value)

        # Pad with leading 0s if necessary
        numstr = '0' * (4 - len(numstr)) + numstr

        # Get ascending and descending integer values
        asc = int(''.join(sorted(numstr)))
        dec = int(''.join(sorted(numstr)[::-1]))

        # Calculate our new value        
        value = dec - asc
        print("Iteration %d: %d" % (n, value))

        # We should always reach the constant within 7 iterations
        if n == 8:
            print("Something went wrong...")
            return -1

    print("Reached 6174 after %d iterations." % n)
    return n

Update

Thanks for the feedback, everyone!

\$\endgroup\$
6
\$\begingroup\$

Validation

  • The range check if value < 1 or value > 9998 could be more Pythonically expressed as if not 1 <= value <= 9998. (I would prefer to avoid hard-coding 9998 altogether, though. See below.)
  • To ensure that there are at least two different digits, you can just use a set.
  • An easier way to zero-pad a number to four places is '{:04d}'.format(value). That line of code is written twice; it may be worthwhile to extract it into a function.
  • Returning None and -1 to indicate validation errors and excessive iterations is unusual for Python. When you encounter an error, raise an exception instead.

Algorithm

  • The implementation presupposes that you know the answer (6174, within 7 iterations). That's a bit dissatisfying at an intellectual level; it feels like a unit test. As an alternative to checking whether the known goal has been reached, you could check whether the sequence has reached a fixed point.
  • You hard-code a lot of special numbers: 4, 9998, 6174, 8. It would be nice to reduce the usage of such constants, and where they are necessary, clarify their purpose. The Wikipedia page mentions that there is a 3-digit Kaprekar Process; it would be nice to have code that is easily adaptable to that related problem.
  • The loop has two purposes: check if the goal has been reached, and count the number of iterations. You have chosen to make the former into the loop termination condition. (It's customary to omit the parentheses there, by the way.) I think that converting it into a counting loop works better, as it brings unity to the thee lines n = 0, n += 1, and if n == 8: ….
  • By postponing the conversion to int when defining asc, you can simplify the derivation of dec a bit.

Suggested implementation

def kaprekar(value):
    def digit_str(n, places):
        return ('{:0%dd}' % places).format(n)

    #PLACES, GOAL, ITERATION_LIMIT = 3, 495, 7
    PLACES, GOAL, ITERATION_LIMIT = 4, 6174, 8
    digits = digit_str(value, PLACES)

    if value <= 0:
        raise ValueError("Input value must be positive.")
    if len(digits) != PLACES:
        raise ValueError("Input value must be %d digits long." % PLACES)
    if len(set(digits)) < 2:
        raise ValueError("Input value must consist of at least two different digits.")

    for iterations in range(ITERATION_LIMIT):
        print("Iteration %d: %s" % (iterations, digits))
        if value == GOAL:
            print("Reached %d after %d iterations." % (GOAL, iterations))
            return iterations

        asc = ''.join(sorted(digits))
        dsc = asc[::-1]

        value = int(dsc) - int(asc)
        digits = digit_str(value, PLACES)
    raise StopIteration("Something went wrong...")

Further enhancement

This version separates the iteration logic from the output routines by using a generator. It also makes no presuppositions about The Answer.

import sys

def kaprekar(digits):
    if int(digits) <= 0:
        raise ValueError("Input value must be positive.")
    if len(set(digits)) < 2:
        raise ValueError("Input value must consist of at least two different digits.")

    places = len(digits)
    prev_digits = None
    while digits != prev_digits:
        yield digits
        prev_digits = digits
        asc = ''.join(sorted(digits))
        dsc = asc[::-1]

        value = int(dsc) - int(asc)
        digits = ('{:0%dd}' % places).format(value)


def main(_, numstr):
    try:
        for iterations, digits in enumerate(kaprekar(numstr)):
            print("Iteration {i}: {d}".format(i=iterations, d=digits))
        print("Reached {d} after {i} iterations".format(i=iterations, d=digits))
    except ValueError as e:
        print(e, file=sys.stderr)
        sys.exit(1)

if __name__ == '__main__':
    main(*sys.argv)
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Thanks for this, very detailed, and I learned a lot. I didn't know about if not 1 <= value <= 9998 in Python, that's pretty cool! But what's the underscore in def main(_, numstr): mean? \$\endgroup\$ – Reticulated Spline Nov 23 '14 at 3:23
  • \$\begingroup\$ @ReticulatedSpline _ is the conventional name for a variable whose value you don't care about. In this case, sys.argv[0] is the name of the program, which is irrelevant. \$\endgroup\$ – 200_success Nov 28 '14 at 16:39
3
\$\begingroup\$

Information

I've written a function (module?)

If it is in a file of it's own it can be considered a module.

Criticism

import collections

def kaprekar(value):

In here you are missing the shebang and also a doc-string for the function

#!/usr/bin/env python
"""kaprekar module

Author :
    Your name

Dependencies :
    collections    
"""

import collections

def kaprekar(value):   
"""kaprekar : iterate the given number till it reaces kaprekar constant

Parameters :
    value - integer to use
"""

Scenario 1

if value < 1 or value > 9998:
    print("Input value must be between 1 and 9998, inclusive.")
    return

Scenario 2

if len(collections.Counter(numstr)) == 1:
    print("Input value must consist of at least two different digits.")
    return

Scenario 3

if n == 8:
    print("Something went wrong...")
    return -1

You should consider returning a meaningful value for each returns. Ex : -1 for errors and 0-7 for successful iterations Or you could simply raise an Exception when something goes wrong.

raise Exception("Exception String")

This is an example, You may use suitable exception type.

numstr = '0' * (4 - len(numstr)) + numstr

You have duplicated this code twice, you should move it into a function of its own.

What you did right

if len(collections.Counter(numstr)) == 1:

I like this part of your code, It's a smart approach. I also like that fact you are failing the function soon as you discover an inconsistency in the input. However you should raise exceptions as I suggested before. Alternatively you may use < 2.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the feedback. I don't have a shebang because this is all under Windows. And thanks for telling me what I did right, at least there was one thing. :) \$\endgroup\$ – Reticulated Spline Nov 23 '14 at 3:24
  • \$\begingroup\$ @ReticulatedSpline : you need one to make your code executable on it's own in *nix. stackoverflow.com/questions/6908143/… \$\endgroup\$ – bhathiya-perera Nov 28 '14 at 8:44
3
\$\begingroup\$

Since it's guaranteed to reach the goal within a number of iterations, I would use an assertion, for example:

# in your original
assert n < 8  # guaranteed by the algorithm

# in 200 success' version
raise AssertionError('Was not supposed to reach this, guaranteed by the algorithm')

Raising any other error would suggest to users that perhaps they should try to catch the exception, but that's not the case here. It's a theoretically impossible situation. The assertion serves as documentation of our assumptions, and it's a safeguard in case something changes unexpectedly. Failing the assertion would be a material defect, and the program should be rewritten.

This code appears twice:

numstr = str(value)

# Pad with leading 0s if necessary
numstr = '0' * (4 - len(numstr)) + numstr

Don't repeat yourself. This should been extracted to a function.

Using unnecessary parentheses like this is not Pythonic:

while (value != 6174):
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I didn't even realize about the parentheses, as I come from a C/C# background and I'm so used to them. \$\endgroup\$ – Reticulated Spline Nov 23 '14 at 3:25
2
\$\begingroup\$

One possible improvement would be to rewrite your code in a more structured and abstract way. You're solving two distinct problems here

  • apply a certain transformation function to a number
  • find out if this transformation has a fixed point

Your program would be cleaner if you code these two functions separately. The first one is the Kaprekar's routine:

LEN = 4

def kaprekar(num):
    s = '{:0{}d}'.format(num, LEN)
    s = ''.join(sorted(s))
    a = int(s)
    b = int(s[::-1])
    return b - a

Note how I'm using the constant LEN instead of 4. First, magic numbers are frowned upon, second, it makes this function suitable for experimenting with other lengths (perhaps, you'll discover your own constant some day!).

The second part is the function that, given an arbitrary function and initial value, calculates the fixed point:

def fixpoint(fn, arg, max_iterations=1000):
    for _ in xrange(max_iterations):
        prev, arg = arg, fn(arg)
        if arg == prev:
            return arg
    raise ValueError('no fixpoint can be reached')

Both these functions don't "know" anything about each other. That makes the program more modular and easier to debug, since you can test each one separately.

Now, the main routine is easy:

 # obtain the argument - left as an exercise
 # check validity      - left as an exercise

 print fixpoint(kaprekar, argument)
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for your feedback. Again I see this underscore being used, this time as an iterator(?) in a loop. What does the underscore mean? \$\endgroup\$ – Reticulated Spline Nov 23 '14 at 3:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.