Skip to main content
Code Review

Return to Answer

deleted 66 characters in body
Source Link
Graipher
  • 41.6k
  • 7
  • 70
  • 134

Review

Python has an official style-guide, PEP8. It recommends using white-space only where necessary (so the blank line after def squarefree(n): could go, as could the trailing space) and using four spaces as indentation.

Normally I would consider it best practice for a function like this to return actually True or False. Your function returns False or a truthy value, which also works, but is not as clean. Consider changing it from return n to return True.


Alternate implementation

As @Dair mentioned in the comments, depending on your use-case for this, it would be faster to write a sieve that gets all square-free numbers up to some limit.

Starting with the Sieve of Eratosthenes, taken from another of my answers, this could be implemented like this:

def squarefree(n):
 """
 Check if n is a square-free number, i.e. is divisible by no other perfect square than 1.
 Args:
 n positive integer to check
 Returns:
 n if n is a square-free number
 False else
 """
 for i in range(2, round(n**0.5) + 1):
 if n % (i**2) == 0:
 return False
 return n
def square_free_sieve(limit):
 """Generator that yields all square free numbers less than limit"""
 a = [True] * limit
 # Needed so we don't mark off multiples of 1^2
 yield 1
 a[0] = a[1] = False
 for i, is_square_free in enumerate(a):
 if is_square_free:
 yield i
 i2 = i * i
 # Start at 2 * i**2 to allow first square free number, like 9
 for n in range(2 * i2, limit, i2):
 a[n] = False
test1 = [n for n in range(100) if squarefree(n)]
test2 = list(square_free_sieve(100))
assert test1 == test2

When generating all square free numbers up to 100,000, your code needs about 3.5s, while the sieve only takes 0.05s, which is about 70 times faster.

Review

Python has an official style-guide, PEP8. It recommends using white-space only where necessary (so the blank line after def squarefree(n): could go, as could the trailing space) and using four spaces as indentation.

Normally I would consider it best practice for a function like this to return actually True or False. Your function returns False or a truthy value, which also works, but is not as clean. Consider changing it from return n to return True.


Alternate implementation

As @Dair mentioned in the comments, depending on your use-case for this, it would be faster to write a sieve that gets all square-free numbers up to some limit.

Starting with the Sieve of Eratosthenes, taken from another of my answers, this could be implemented like this:

def squarefree(n):
 """
 Check if n is a square-free number, i.e. is divisible by no other perfect square than 1.
 Args:
 n positive integer to check
 Returns:
 n if n is a square-free number
 False else
 """
 for i in range(2, round(n**0.5)):
 if n % (i**2) == 0:
 return False
 return n
def square_free_sieve(limit):
 """Generator that yields all square free numbers less than limit"""
 a = [True] * limit
 # Needed so we don't mark off multiples of 1^2
 yield 1
 a[0] = a[1] = False
 for i, is_square_free in enumerate(a):
 if is_square_free:
 yield i
 i2 = i * i
 # Start at 2 * i**2 to allow first square free number, like 9
 for n in range(2 * i2, limit, i2):
 a[n] = False
test1 = [n for n in range(100) if squarefree(n)]
test2 = list(square_free_sieve(100))
assert test1 == test2

When generating all square free numbers up to 100,000, your code needs about 3.5s, while the sieve only takes 0.05s, which is about 70 times faster.

Review

Python has an official style-guide, PEP8. It recommends using white-space only where necessary (so the blank line after def squarefree(n): could go, as could the trailing space) and using four spaces as indentation.

Normally I would consider it best practice for a function like this to return actually True or False. Your function returns False or a truthy value, which also works, but is not as clean. Consider changing it from return n to return True.


Alternate implementation

As @Dair mentioned in the comments, depending on your use-case for this, it would be faster to write a sieve that gets all square-free numbers up to some limit.

Starting with the Sieve of Eratosthenes, taken from another of my answers, this could be implemented like this:

def squarefree(n):
 """
 Check if n is a square-free number, i.e. is divisible by no other perfect square than 1.
 Args:
 n positive integer to check
 Returns:
 n if n is a square-free number
 False else
 """
 for i in range(2, round(n**0.5) + 1):
 if n % (i**2) == 0:
 return False
 return n
def square_free_sieve(limit):
 """Generator that yields all square free numbers less than limit"""
 a = [True] * limit
 # Needed so we don't mark off multiples of 1^2
 yield 1
 a[0] = a[1] = False
 for i, is_square_free in enumerate(a):
 if is_square_free:
 yield i
 i2 = i * i
 for n in range(i2, limit, i2):
 a[n] = False
test1 = [n for n in range(100) if squarefree(n)]
test2 = list(square_free_sieve(100))
assert test1 == test2

When generating all square free numbers up to 100,000, your code needs about 3.5s, while the sieve only takes 0.05s, which is about 70 times faster.

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Review

Python has an official style-guide, PEP8. It recommends using white-space only where necessary (so the blank line after def squarefree(n): could go, as could the trailing space) and using four spaces as indentation.

Normally I would consider it best practice for a function like this to return actually True or False. Your function returns False or a truthy value, which also works, but is not as clean. Consider changing it from return n to return True.


Alternate implementation

As @Dair @Dair mentioned in the comments, depending on your use-case for this, it would be faster to write a sieve that gets all square-free numbers up to some limit.

Starting with the Sieve of Eratosthenes, taken from another of my answers my answers, this could be implemented like this:

def squarefree(n):
 """
 Check if n is a square-free number, i.e. is divisible by no other perfect square than 1.
 Args:
 n positive integer to check
 Returns:
 n if n is a square-free number
 False else
 """
 for i in range(2, round(n**0.5)):
 if n % (i**2) == 0:
 return False
 return n
def square_free_sieve(limit):
 """Generator that yields all square free numbers less than limit"""
 a = [True] * limit
 # Needed so we don't mark off multiples of 1^2
 yield 1
 a[0] = a[1] = False
 for i, is_square_free in enumerate(a):
 if is_square_free:
 yield i
 i2 = i * i
 # Start at 2 * i**2 to allow first square free number, like 9
 for n in range(2 * i2, limit, i2):
 a[n] = False
test1 = [n for n in range(100) if squarefree(n)]
test2 = list(square_free_sieve(100))
assert test1 == test2

When generating all square free numbers up to 100,000, your code needs about 3.5s, while the sieve only takes 0.05s, which is about 70 times faster.

Review

Python has an official style-guide, PEP8. It recommends using white-space only where necessary (so the blank line after def squarefree(n): could go, as could the trailing space) and using four spaces as indentation.

Normally I would consider it best practice for a function like this to return actually True or False. Your function returns False or a truthy value, which also works, but is not as clean. Consider changing it from return n to return True.


Alternate implementation

As @Dair mentioned in the comments, depending on your use-case for this, it would be faster to write a sieve that gets all square-free numbers up to some limit.

Starting with the Sieve of Eratosthenes, taken from another of my answers, this could be implemented like this:

def squarefree(n):
 """
 Check if n is a square-free number, i.e. is divisible by no other perfect square than 1.
 Args:
 n positive integer to check
 Returns:
 n if n is a square-free number
 False else
 """
 for i in range(2, round(n**0.5)):
 if n % (i**2) == 0:
 return False
 return n
def square_free_sieve(limit):
 """Generator that yields all square free numbers less than limit"""
 a = [True] * limit
 # Needed so we don't mark off multiples of 1^2
 yield 1
 a[0] = a[1] = False
 for i, is_square_free in enumerate(a):
 if is_square_free:
 yield i
 i2 = i * i
 # Start at 2 * i**2 to allow first square free number, like 9
 for n in range(2 * i2, limit, i2):
 a[n] = False
test1 = [n for n in range(100) if squarefree(n)]
test2 = list(square_free_sieve(100))
assert test1 == test2

When generating all square free numbers up to 100,000, your code needs about 3.5s, while the sieve only takes 0.05s, which is about 70 times faster.

Review

Python has an official style-guide, PEP8. It recommends using white-space only where necessary (so the blank line after def squarefree(n): could go, as could the trailing space) and using four spaces as indentation.

Normally I would consider it best practice for a function like this to return actually True or False. Your function returns False or a truthy value, which also works, but is not as clean. Consider changing it from return n to return True.


Alternate implementation

As @Dair mentioned in the comments, depending on your use-case for this, it would be faster to write a sieve that gets all square-free numbers up to some limit.

Starting with the Sieve of Eratosthenes, taken from another of my answers, this could be implemented like this:

def squarefree(n):
 """
 Check if n is a square-free number, i.e. is divisible by no other perfect square than 1.
 Args:
 n positive integer to check
 Returns:
 n if n is a square-free number
 False else
 """
 for i in range(2, round(n**0.5)):
 if n % (i**2) == 0:
 return False
 return n
def square_free_sieve(limit):
 """Generator that yields all square free numbers less than limit"""
 a = [True] * limit
 # Needed so we don't mark off multiples of 1^2
 yield 1
 a[0] = a[1] = False
 for i, is_square_free in enumerate(a):
 if is_square_free:
 yield i
 i2 = i * i
 # Start at 2 * i**2 to allow first square free number, like 9
 for n in range(2 * i2, limit, i2):
 a[n] = False
test1 = [n for n in range(100) if squarefree(n)]
test2 = list(square_free_sieve(100))
assert test1 == test2

When generating all square free numbers up to 100,000, your code needs about 3.5s, while the sieve only takes 0.05s, which is about 70 times faster.

added 165 characters in body; added 145 characters in body; deleted 46 characters in body; added 1 character in body
Source Link
Graipher
  • 41.6k
  • 7
  • 70
  • 134

Review

Python has an official style-guide, PEP8. It recommends using white-space only where necessary (so the blank line after def squarefree(n): could go, as could the trailing space) and using four spaces as indentation.

Normally I would consider it best practice for a function like this to return actually True or False. Your function returns False or a truthy value, which also works, but is not as clean. Consider changing it from return n to return True.


Alternate implementation

As @Dair mentioned in the comments, depending on your use-case for this, it would be faster to write a sieve that gets all square-free numbers up to some limit.

Starting with the Sieve of Eratosthenes, taken from another of my answers, this could be implemented like this:

def squarefree(n):
 """
 Check if n is a square-free number, i.e. is divisible by no other perfect square than 1.
 Args:
 n positive integer to check
 Returns:
 n if n is a square-free number
 False else
 """
 for i in range(2, round(n**0.5)):
 if n % (i**2) == 0:
 return False
 return n
def square_free_sieve(limit):
 """Generator that yields all square free numbers less than limit"""
 a = [True] * limit
 # Needed so we don't mark off multiples of 1^2
 yield 1
 a[0] = a[1] = False
 for i, is_square_free in enumerate(a):
 if is_square_free:
 yield i
 i2 = i * i
 # Start at 2 * i**2 to allow first square free number, like 9
 for n in range(2 * i2, limit, i2):
 a[n] = False
test1 = [n for n in range(100) if squarefree(n)]
test2 = list(square_free_sieve(100))
assert test1 == test2

When generating all square free numbers up to 100,000, your code needs about 3.5s, while the sieve only takes 0.05s, which is about 70 times faster.

Review

Python has an official style-guide, PEP8. It recommends using white-space only where necessary (so the blank line after def squarefree(n): could go, as could the trailing space) and using four spaces as indentation.

Normally I would consider it best practice for a function like this to return actually True or False. Your function returns False or a truthy value, which also works, but is not as clean. Consider changing it from return n to return True.


Alternate implementation

As @Dair mentioned in the comments, depending on your use-case for this, it would be faster to write a sieve that gets all square-free numbers up to some limit.

Starting with the Sieve of Eratosthenes, taken from another of my answers, this could be implemented like this:

def squarefree(n):
 for i in range(2, round(n**0.5)):
 if n % (i**2) == 0:
 return False
 return n
def square_free_sieve(limit):
 """Generator that yields all square free numbers less than limit"""
 a = [True] * limit
 # Needed so we don't mark off multiples of 1^2
 yield 1
 a[0] = a[1] = False
 for i, is_square_free in enumerate(a):
 if is_square_free:
 yield i
 i2 = i * i
 # Start at 2 * i**2 to allow first square free number, like 9
 for n in range(2 * i2, limit, i2):
 a[n] = False
test1 = [n for n in range(100) if squarefree(n)]
test2 = list(square_free_sieve(100))
assert test1 == test2

When generating all square free numbers up to 100,000, your code needs about 3.5s, while the sieve only takes 0.05s, which is about 70 times faster.

Review

Python has an official style-guide, PEP8. It recommends using white-space only where necessary (so the blank line after def squarefree(n): could go, as could the trailing space) and using four spaces as indentation.

Normally I would consider it best practice for a function like this to return actually True or False. Your function returns False or a truthy value, which also works, but is not as clean. Consider changing it from return n to return True.


Alternate implementation

As @Dair mentioned in the comments, depending on your use-case for this, it would be faster to write a sieve that gets all square-free numbers up to some limit.

Starting with the Sieve of Eratosthenes, taken from another of my answers, this could be implemented like this:

def squarefree(n):
 """
 Check if n is a square-free number, i.e. is divisible by no other perfect square than 1.
 Args:
 n positive integer to check
 Returns:
 n if n is a square-free number
 False else
 """
 for i in range(2, round(n**0.5)):
 if n % (i**2) == 0:
 return False
 return n
def square_free_sieve(limit):
 """Generator that yields all square free numbers less than limit"""
 a = [True] * limit
 # Needed so we don't mark off multiples of 1^2
 yield 1
 a[0] = a[1] = False
 for i, is_square_free in enumerate(a):
 if is_square_free:
 yield i
 i2 = i * i
 # Start at 2 * i**2 to allow first square free number, like 9
 for n in range(2 * i2, limit, i2):
 a[n] = False
test1 = [n for n in range(100) if squarefree(n)]
test2 = list(square_free_sieve(100))
assert test1 == test2

When generating all square free numbers up to 100,000, your code needs about 3.5s, while the sieve only takes 0.05s, which is about 70 times faster.

added 75 characters in body
Source Link
Graipher
  • 41.6k
  • 7
  • 70
  • 134
Loading
added 27 characters in body
Source Link
Graipher
  • 41.6k
  • 7
  • 70
  • 134
Loading
added 120 characters in body
Source Link
Graipher
  • 41.6k
  • 7
  • 70
  • 134
Loading
Source Link
Graipher
  • 41.6k
  • 7
  • 70
  • 134
Loading
lang-py

AltStyle によって変換されたページ (->オリジナル) /