5
\$\begingroup\$

I wrote some basic math functions:

def is_multiple_of(x: float, y: float):
    """Checks if x is a multiple of y and returns a boolean."""
    return x / y == int(x / y)

def get_even(iterable, is_sorted: bool, return_tuple: bool):
    """Gets all the even numbers in a list/tuple and returns another list/tuple."""
    even_list = []
    for obj in iterable:
        even_list.append(obj)
        if not is_multiple_of(obj, 2):
            even_list.remove(obj)
    even_list = sorted(even_list) if is_sorted else even_list
    return even_list if not return_tuple else tuple(even_list)

def get_odd(iterable, is_sorted: bool, return_tuple: bool):
    """Gets all the odd numbers in a list/tuple and returns another list/tuple."""
    odd_list = []
    for obj in iterable:
        odd_list.append(obj)
        if is_multiple_of(obj, 2):
            odd_list.remove(obj)
    odd_list = sorted(odd_list) if is_sorted else odd_list
    return odd_list if not return_tuple else tuple(odd_list)
    
def get_uniques(iterable, is_sorted: bool, return_tuple: bool):
    """Gets all the unique variables in a list/tuple and returns another list/tuple."""
    uniques = []
    for obj in iterable:
        if not obj in uniques:
            uniques.append(obj)
    uniques = sorted(uniques) if is_sorted else uniques
    return uniques if not return_tuple else tuple(uniques)

Usage:

print(is_multiple_of(14, 7))

True

print(get_even([1, 2, 3, 4, 5, 6, 7, 8, 9, 10], False))

[2, 4, 6, 8, 10]

print(get_odd([1, 2, 3, 4, 5, 6, 7, 8, 9, 10], True))

(1, 3, 5, 7, 9)

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! I have rolled back Rev 5 → 4. Please see What should I do when someone answers my question?. \$\endgroup\$ Commented Jun 13 at 22:16
  • \$\begingroup\$ To me, is_sorted says whether the input is sorted. If I want to optionally toggle the output, I would have the parameter named should_sort. But I agree with Toby Speight that the boolean options should be removed. \$\endgroup\$ Commented Jun 16 at 13:22

4 Answers 4

9
\$\begingroup\$

Failure with float inputs

is_multiple_of(1.0, 1/3) wrongly returns True. The value of y is 0.333333333333333314829616256247390992939472198486328125, and 1.0 is no multiple of that.

The easiest fix of that case is to remove the false advertising. As Chris said, declare the parameters int. That seems what you had in mind anyway, since your later "if it's not a multiple of 2, then it's odd" is only valid for integers. And if you declare them int, then the above failure example isn't your fault but mine, for using the function with invalid arguments.

Failure with int inputs

But even with int inputs, it still fails for example is_multiple_of(10000000000000001, 2), returning True. Because it unsafely calculates with float values. Better use just int and check whether dividing x by y leaves no remainder:

def is_multiple_of(x: int, y: int) -> bool:
    """Return whether x is a multiple of y."""
    if not y:
        return not x
    return not x % y

Docstring conventions

Note I also rewrote the docstring according to PEP 257 – Docstring Conventions:

prescribes the function or method’s effect as a command (“Do this”, “Return that”), not as a description; e.g. don’t write “Returns the pathname …”

The one-line docstring should NOT be a “signature” reiterating the function/method parameters (which can be obtained by introspection).

Strictly speaking, the latter is about docstrings like """function(a, b) -> list""", and shows """Do X and return a list.""" as a good docstring. But I think that's because the PEP is old and doesn't have type annotations in mind. If you declare -> bool, then I'd say in the spirit of the PEP's guideline, the docstring shouldn't reiterate that.

Properly supporting float inputs

But we can also support float arguments properly. The simplest way is to use Fraction, which gives us exact calculations:

from fractions import Fraction

def is_multiple_of(x: float, y: float) -> bool:
    """Return whether x is a multiple of y."""
    try:
        x = Fraction(x)
        y = Fraction(y)
    except (OverflowError, ValueError):
        return False
    if not y:
        return not x
    return not x % y

The OverflowError and ValueError are for infinite and "not a number" floats.

A few more ways:

def is_multiple_of(x: float, y: float) -> bool:
    """Return whether x is a multiple of y."""
    try:
        return not Fraction(x) % Fraction(y)
    except ZeroDivisionError:
        return not x
    except (OverflowError, ValueError):
        return False
import math

def is_multiple_of(x: float, y: float) -> bool:
    """Return whether x is a multiple of y."""
    if not math.isfinite(x) or not math.isfinite(y):
        return False
    if not y:
        return not x
    a, b = x.as_integer_ratio()
    c, d = y.as_integer_ratio()
    # Checking whether a/b is a multiple of c/d is equivalent 
    # to checking whether ad is a multiple of cb.
    return not (a*d) % (c*b)
def is_multiple_of(x: float, y: float) -> bool:
    """Return whether x is a multiple of y."""
    if not math.isfinite(x) or not math.isfinite(y):
        return False
    if not y:
        return not x
    a, b = x.as_integer_ratio()
    c, d = y.as_integer_ratio()
    if b > d:
        return False
    return not a % c

The last one uses that the denominators are powers of 2.

\$\endgroup\$
8
  • \$\begingroup\$ Sorry. I will respond will a fix soon. \$\endgroup\$ Commented Jun 13 at 23:02
  • \$\begingroup\$ This is looking to be a lot more difficult than I thought. I promise to respond with a solution when I find one. \$\endgroup\$ Commented Jun 13 at 23:40
  • \$\begingroup\$ I think it's fixed: def is_multiple_of(x: float, y: float): """Checks if x is a multiple of y and returns a boolean.""" try: return y / x == y // x except ZeroDivisionError: return "Y cannot be 0." \$\endgroup\$ Commented Jun 13 at 23:53
  • \$\begingroup\$ @LoatheCode That claims that 2 is not a multiple of 1. \$\endgroup\$ Commented Jun 14 at 0:32
  • \$\begingroup\$ Could this work? def is_multiple_of(x: float, y: float): """Checks if x is a multiple of y and returns a boolean.""" try: return y / x == y // x if y > x else x / y == x // y except ZeroDivisionError: return "Y cannot be 0." \$\endgroup\$ Commented Jun 14 at 3:49
7
\$\begingroup\$

Setting aside issues with your is_multiple_of function, consider your get_even function.

def get_even(iterable, is_sorted: bool, return_tuple: bool):
    """Gets all the even numbers in a list/tuple and returns another list/tuple."""
    even_list = []
    for obj in iterable:
        even_list.append(obj)
        if not is_multiple_of(obj, 2):
            even_list.remove(obj)
    even_list = sorted(even_list) if is_sorted else even_list
    return even_list if not return_tuple else tuple(even_list)

Unnecessary work

Appending a value from iterable to even_list and then immediately removing it again if it is not a multiple of 2 is unnecessary work when we can just conditionally append in the first place.

def get_even(iterable, is_sorted: bool, return_tuple: bool):
    """Gets all the even numbers in a list/tuple and returns another list/tuple."""
    even_list = []
    for obj in iterable:
        if is_multiple_of(obj, 2):
            even_list.append(obj)
    even_list = sorted(even_list) if is_sorted else even_list
    return even_list if not return_tuple else tuple(even_list)

List comprehensions

But this can really be expressed as a list comprehension.

def get_even(iterable, is_sorted: bool, return_tuple: bool):
    """Gets all the even numbers in a list/tuple and returns another list/tuple."""
    even_list = [obj for obj in iterable if is_multiple_of(obj, 2)]
    even_list = sorted(even_list) if is_sorted else even_list
    return even_list if not return_tuple else tuple(even_list)

Conditional expressions vs. statements

Your use of conditional expressions also feels unnecessarily complex vs just using conditional statements. And at the same time, some whitespace doesn't go amiss.

def get_even(iterable, is_sorted: bool, return_tuple: bool):
    """Gets all the even numbers in a list/tuple and returns another list/tuple."""

    even_list = [obj for obj in iterable if is_multiple_of(obj, 2)]

    if is_sorted: 
        even_list.sort()

    if return_tuple:
        return tuple(even_list)
    else:
        return even_list

The same critique would go towards get_odd.

Why float at all?

Your is_mutliple_of function is specifically annotated to work on float values, but you never show these being used, and in the context of get_even and get_odd it makes no sense. What is an "odd" or "even" floating point number?

I would recommend replacing this function call with a much simpler test of the remainder using %:

def get_even(iterable, is_sorted: bool, return_tuple: bool):
    """Gets all the even numbers in a list/tuple and returns another list/tuple."""

    even_list = [obj for obj in iterable if obj % 2 == 0]

    if is_sorted: 
        even_list.sort()

    if return_tuple:
        return tuple(even_list)
    else:
        return even_list

Returning an iterable and keeping your functions focussed

Your get_even and get_odd functions take an iterable value, so maybe have them return an iterable. Having arguments to decide whether to sort the output or return it as a tuple is asking this function to do a lot. Instead let the caller decide how to use the returned values.

def get_even(iterable):
    for obj in iterable:
        if obj % 2 == 0:
            yield obj
\$\endgroup\$
0
6
\$\begingroup\$

A function should do one thing, and do it well. These functions pile on additional responsibility, as evidenced by their signatures:

def get_even(iterable, is_sorted: bool, return_tuple: bool):
def get_odd(iterable, is_sorted: bool, return_tuple: bool):
def get_uniques(iterable, is_sorted: bool, return_tuple: bool):

And we've duplicated the is_sorted and return_tuple logic in each function.

If we remove those arguments, then we can let the callers compose the operations they need (e.g. sorted(get_even(…)) or tuple(sorted(get_uniques(…))).

The first two functions simply filter an input list, and there's a built-in filter function to do that. We just need to provide suitable predicates:

def is_even(n: int) -> bool
    return n % 2 == 0

def is_odd(n: int) -> bool
    return n % 2 != 0

Then get_even(input) goes away, because it's just filter(is_even, input).

get_uniques is badly named - it seems to just eliminate duplicates (i.e. it returns the distinct values, not "unique variables"). Which we can do by simply constructing a set from the inputs.

If we really did want to find unique values - meaning those which appear exactly once in the input - then we could construct a collections.Counter and select items from that:

from collections import Counter
def uniques(values):
    return [x for x,n in Counter(values).items() if n == 1]

Certainly, the code in get_uniques() scales badly, as the linear obj in uniques test will get slower and slower as that list grows.

\$\endgroup\$
2
  • \$\begingroup\$ Are unique, unique_justseen and unique_everseen from Python's Itertools Recipes also badly named then? I think the word "unique" is ambiguous and the way they mean it is legitimate and common. \$\endgroup\$ Commented Jun 14 at 10:11
  • \$\begingroup\$ I think those names stem from the uniq utility, which exists to "uniquify" its input stream (i.e. make unique, rather than to find unique entries). Perhaps make_unique() is more unambiguous than get_uniques()? \$\endgroup\$ Commented Jun 14 at 15:26
5
\$\begingroup\$

Input checking

It would be helpful to check for the common "divide-by-0" error in the is_multiple_of function. One way is to use try/except to gracefully handle the case where the y input is 0.

There is a built-in ZeroDivisionError exception.

Type hints

You added a helpful type hint for the function inputs. You could also add type hints for the return value of is_multiple_of:

def is_multiple_of(x: float, y: float) -> bool:

\$\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.