13
\$\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\$
0

5 Answers 5

8
\$\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)
\$\endgroup\$
4
  • 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\$ Commented Nov 23, 2014 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\$ Commented Nov 28, 2014 at 16:39
  • \$\begingroup\$ There should be a check that the input comprises 4 digits \$\endgroup\$
    – boardrider
    Commented Nov 24, 2022 at 0:18
  • \$\begingroup\$ For instance, add: ` if (4 != len(digits)):` ` raise ValueError("Input value must consist of four digits.")` \$\endgroup\$
    – boardrider
    Commented Nov 24, 2022 at 0:24
5
\$\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)
\$\endgroup\$
2
  • 2
    \$\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\$ Commented Nov 23, 2014 at 3:25
  • \$\begingroup\$ @ReticulatedSpline just a convention, it means the variable _ (a normal variable like any other) is not going to be used. \$\endgroup\$
    – Caridorc
    Commented Jan 8, 2022 at 18:56
4
\$\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):
\$\endgroup\$
1
  • \$\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\$ Commented Nov 23, 2014 at 3:25
4
\$\begingroup\$

Information

I've written a function (module?)

If it is in a file of its 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 reaches 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. E.g. -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.

\$\endgroup\$
2
  • 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\$ Commented Nov 23, 2014 at 3:24
  • \$\begingroup\$ @ReticulatedSpline : you need one to make your code executable on it's own in *nix. stackoverflow.com/questions/6908143/… \$\endgroup\$
    – JaDogg
    Commented Nov 28, 2014 at 8:44
1
\$\begingroup\$

Apart from the existing suggestion to raise errors (I've stuck with AssertionErrors to keep things simple for now), I noted three points of interest: the string padding was explicitly coded twice, we can DRY the difference between two sorts as shown below, and itertools.count provides a better option than a while loop that manually increments:

from itertools import count

def long_string(value):
    short_numstr = str(value)
    return '0'*(4-len(short_numstr))+short_numstr

def kaprekar(value):   
    assert 1 <= value <= 9998, f"{value} not between 1 and 9998, inclusive."
    assert len(set(long_string(value))) > 1, f"{value} consists of fewer than two different digits."
    original = value
    for n in count(1):
        numstr = sorted(long_string(value))
        value = int(numstr[::-1])-int(numstr)
        if value == 6174: return n
        assert n < 8, f"Something went wrong with {original}..."

(I've also edited the messages to reference which value caused the problem.) As of Python 3.8, the walrus operator allows a slight line count reduction not used above.

\$\endgroup\$

Your Answer

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

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