I have often read about how important clear and efficient code is. Also often people talk and write about 'beautiful' code. Some hints and critic from experienced developers for the following code would be extremely helpful to me. Another question that I often ask myself is about list comprehensions: I feel that in the first specific code (about prime palindromes), especially the second function that verifies palindrome characteristics could be expressed on one line in a list comprehension - how would that work, and would it even be an advantage of some sort?

The code calculates all the prime palindromes from 0 to 1000 and prints the largest.

#My solution to codeval challenge Prime Palindrome. AUTHOR: S.Spiess
#initial count
counting_list = [x for x in range(0,1001)]

#prime number check. make list prime_list containing only primes from 0 to 1000
def prime_check(a_list):
    prime_list = []
    for k in a_list:
        count = 2.0
        d_test = 0
        while count < k:
            if k % count == 0:
                d_test += 1
                count += 1
            else:
                count += 1
        if d_test < 1 and k > 1:
            prime_list.append(k)

    return prime_list

#check prime numbers from previous function for palindrome characteristic. append in new list.
def palindrome_check(num_list):
    palindrome_list = []
    for i in num_list:
        temp = str(i)
        if temp == temp[::-1]:
            palindrome_list.append(i)

    return palindrome_list

#print biggest palindrome prime from 0 to 1000
print max(palindrome_check(prime_check(counting_list)))

Here is another sample of code I wrote. It contains two functions that can change the base of a number.

def to_mod20(any_num):
    a_list = []

    if any_num < 20 and any_num >= 1:
        a_list.append(int(any_num))

    while any_num >= 20:
        a_list.append(any_num % 20)
        if any_num / 20 < 20:
            a_list.append(any_num / 20)
        any_num = any_num / 20
    #invert list for proper output
    return a_list[::-1]


def decimal_mod20(any_dec):
    count = 0
    a_list = []
    while any_dec < 1 and count < 4:
        a_list.append(int(any_dec * 17.0))
        any_dec = any_dec * 17.0 - int(any_dec * 17.0)
        count += 1
        #print any_dec

    return a_list
up vote 5 down vote accepted
  • The list comprehension [x for x in range(0,1001)] is redundant, as range already returns a list in Python 2. In Python 3 one can use list(range(1001))
  • Write a docstring instead of a comment to describe a function.
  • Why is count a float (2.0) in prime_check? Should be an int.
  • Prefer for loops to while loops when they do the same thing. Eg. use for count in xrange(2, k) in prime_check.
  • if any_num < 20 and any_num >= 1: can be written as if 1 <= any_num < 20:
  • You may want to look for more efficient algorithms for generating primes.
  • Good to know.. very helpful points! Unfortunately I cannot vote up yet due to missing reputation. – anaheim Apr 29 '13 at 9:55

You "main" program should be like this:

print list(p for p in primes(100000) if is_palindrome(p))

where primes is a generator that yields primes one by one up to the limit, and is_palindrome is a boolean function that takes one argument (of any type) and returns true if its string representation is a palindrome:

def is_palindrome(x):
    return str(x) == str(x)[::-1]

Very few things occur to me here:

  • PEP 8, the Python Style Guide and PEP 257, Python Docstring Conventions. The comments you wrote above functions should become docstrings instead.

  • [x for x in range(0,1001)] is the same as list(range(0, 1001)) which is the same as range(0, 1001). That is, until you actually follow my last point, where the behaviour of range() rather follows that of xrange().

  • You are passing along lists, which implies building them, using them and then throwing them away. In some cases, using generator expressions would be more efficient.

  • You are building a list of palindrome primes of which you then print the maximum. Since the input array is sorted, isn't the output array sorted, too, so that the last one is the maximum? BTW: This lacks a check whether that list is empty! At least formally, since 1 is prime and a palindrome.

  • Last point: Use Python 3! ;)

  • Thanks for your advice. Unfortunately I cannot vote up yet due to missing reputation. – anaheim Apr 29 '13 at 9:56
  • Most mathematicians would say that 1 is neither prime nor composite. – Snowbody Jul 8 '14 at 2:05

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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