3
\$\begingroup\$

So, here's a program in python to get the count of digits that divide the number. If you give the program a list like [24,10245], you get an output of:

2
2

This is the code:

def count_div_digits(l):
    for number in l:
        count=0
        digits=str(number)
        for digit in digits:
            if digit=='0':
                continue
            else:
                if number%(int(digit))==0:
                    count+=1
        print count

The above code obviously works. How can I further optimize this, make it short or do it differently?

\$\endgroup\$
2
  • \$\begingroup\$ Aside: you get the wrong answer for zero, assuming you mean for 0 to be one of its digits (since zero divides zero). \$\endgroup\$ Commented Sep 21, 2015 at 15:36
  • \$\begingroup\$ Depending on whether you consider 0/0 == 0 you should have a case to handle that possibility. Either raise an error or just return 0. \$\endgroup\$ Commented Sep 21, 2015 at 15:51

4 Answers 4

6
\$\begingroup\$

Do one thing

Let's start with your top level:

def count_div_digits(l):
    for number in l:

Firstly, l is a terrible variable name. It looks like 1. Avoid it at all costs. Secondly, this function is less useful than it could be simply because it requires a list of numbers. It'd be better if the function took a single number, then you could call it on a list separately if you want:

def count_div_digits(number):
    ...

for number in lst:
    count_div_digits(number)

Return, don't print

Right now, your count_div_digits simply prints its result. That's all well and good, but it'd be better if you just returned the number. If you want to print it, you can always just do:

print count_div_digits(number)

And this way, you can store the results somewhere else for further analysis later.

Avoid continue:

The less weird hops your loops make, the easier it is to follow along the logic. Right now you have:

if digit=='0':
    continue
else:    
    # stuff

But it'd be clearer to just do:

if digit != '0':
    # same stuff

Proposed Solution

def count_div_digits(number):
    count = 0
    for digit in str(number):
        if digit != '0' and number % int(digit) == 0:
            count += 1
    return count

Generator Expressions

You could also write the above with the help of a generator expression by factoring out a length:

def sizeof(iterable):
    return sum(1 for _ in iterable)

def count_div_digits(number):
    return sizeof(digit for digit in str(number)
                  if digit != '0' and number % int(digit) == 0)

Whether you consider that an improvement or not is a matter of opinion.

\$\endgroup\$
2
  • \$\begingroup\$ Wait, isn't that just the same as len(iterable)? \$\endgroup\$ Commented Sep 21, 2015 at 18:01
  • \$\begingroup\$ Ah I didn't properly read what you were passing to it, makes sense now. \$\endgroup\$ Commented Sep 21, 2015 at 18:04
3
\$\begingroup\$

Single letter names should be used sparingly, especially l as it can be mistaken for uppercase i or the number one. Also you should add spaces either side of the arithmetic operators you're using.

You're just printing the count result. If you're printing a result of a calculation, at the very least wrap it in some text or print the input alongside it so the user can parse what the number means:

print ("{} result: {}".format(number, count))

But what if you want to use the count for something? Generally speaking, printing the result of a calculation is often throwing away work. You should return a list of the counts, and then you could print that returned list if that's all you want.

You could also simplify your loop using the built in sum function with a generator expression. A generator is like a for loop collapsed into a one line expression. Here's how it could look:

digits = str(number)
count = sum(number % (int(digit)) == 0 for digit in digits) 

What this does is evaluates each number like your for loop and creates a boolean value. However it also gets the sum result of all of these booleans. Python can coerce a boolean to an integer (where False is 0 and True is 1), so you'll get the sum result you want. Except that you also need to ignore when it's 0. You can do that by adding an if at the end of your generator expression, and if that condition evaluates as False then Python skips that value.

count = sum(number % (int(digit)) == 0 for digit in digits if number != '0') 

Though I'd split that over two lines as you should try keep to a 79 character limit. Now that we're doing this, I'd personally think it's better to make digits a list of the integers, rather than converting them in your calculation. It would involve a list comprehension, which is a lot like a generator expression above but it creates a full list of the values. You just need to turn the integer into a string, then that string into a list and then each element of the list into an integer.

digits = [int(i) for i in list(str(number))]

This gives you the count in one line, and you could condense the function to only a few lines:

def count_div_digits(nums):
    results = []
    for number in nums:
        digits = [int(i) for i in list(str(number))]
        results.append(sum(number % digit == 0 for digit in digits
                           if number != 0))
    return results
\$\endgroup\$
3
\$\begingroup\$

Your function count_div_digits is responsible for three separate things:

  1. Iterating over a list of numbers;
  2. Calculating the number of digit divisors in that number; and
  3. Displaying the result.

It would be better "separation of concerns" to split that up into two functions:

def process(num_list):
    for number in num_list:
        print count_div_digits(number)

def count_div_digits(number):
    ...

i.e. moving tasks 1. and 3. into a separate function.

This allows count_div_digits to focus on the one thing it needs to do: count and return the number of digits in a number that are divisors of that number. This also makes testing much easier, as we can now e.g. assert count_div_digits(24) == 2 without having to try to access what's getting printed.

Note also that I've used a more descriptive variable name than l!


Python has a style guide that you should follow - removing the unnecessary parentheses and adding the appropriate whitespace gives the code more "room to breathe":

def count_div_digits(number):
    count = 0
    digits = str(number)
    for digit in digits:
        if digit == '0':
            continue
        else:
            if number % int(digit) == 0:
                count += 1
    return count

I would be inclined to add a docstring to count_div_digits at least. You can also use this to include simple doctests (I have added these so that I could refactor and ensure everything still worked):

def count_div_digits(number):
    """Return the count of digits in the number that are its divisors.

    Examples:

        >>> count_div_digits(24)
        2
        >>> count_div_digits(10245)
        2

    """
    ...

In terms of refactoring, note that:

  • You never use digits again, so could simplify to for digit in str(number):
  • Rather than:

    if digits == '0': 
        continue
    else:
        ... 
    

    you could flip the conditional around and skip the else block entirely, leaving:

    if digits != '0':
        ... 
    
  • You can use sum with a generator expression, rather than initialising count = 0 and having a standard for loop - both of the conditionals can be easily incorporated

This leaves you with:

def count_div_digits(number):
    """Return the count of digits in the number that are its divisors.

    ...

    """
    return sum(1 if number % int(digit) == 0 else 0
               for digit in str(number)
               if digit != '0')
\$\endgroup\$
1
  • \$\begingroup\$ Good comments, I would only toss in that either the docstrings need to mention that this is specific to the decimal representation of integers, or the functions should be changed to accept an argument specifying the base to work in. \$\endgroup\$ Commented Sep 21, 2015 at 18:30
2
\$\begingroup\$

I am surprised nobody has noted that all the conversion to/from int is superfluous.

Although I see that in SO Python answers to the same question always do that, contrary to eg. C answer. Is it intentional, is converting to str and doing string operations really preferrable to plain old math?

def count_div_digits(number):
    divisor_digits=set()
    number_copy = number
    while (number_copy > 0):
        digit = number_copy % 10
        if digit !=0 and number % digit == 0:
            divisor_digits.add(digit)
        number_copy /= 10
    return divisor_digits
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.