Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

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)))

I updated my my algorithm for calculating primes, let me know what I could improve!

def hoop(n):
    import math
    sieve = [False, False] + [True] * (n-1)
    for count in xrange(2, int(math.sqrt(n))+1):
        if sieve[count]:
            for m in xrange(2*count, n+1, count):
                sieve[m] = False
    return [i for i, s in enumerate(sieve) if s]
share|improve this question

3 Answers

up vote 3 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.
share|improve this answer
Good to know.. very helpful points! Unfortunately I cannot vote up yet due to missing reputation. – anaheim Apr 29 at 9:55
up vote 4 down vote

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]
share|improve this answer
up vote 1 down vote

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! ;)

share|improve this answer
Thanks for your advice. Unfortunately I cannot vote up yet due to missing reputation. – anaheim Apr 29 at 9:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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