21
\$\begingroup\$

I am trying to solve the following problem:

Given three numbers, a, b, and c, return their product, but if one of the numbers is the same as another, it does not count. If two numbers are similar, return the lonely number. If all numbers are same, return 1.

I have the following solution:

def solve(a, b, c):
    product = 0

    if a == b == c:
        product = 1
    elif a == b:
        product = c
    elif a == c:
        product = b
    elif b == c:
        product = a
    else:
        product = a * b * c
    return product

How can I do this better? Preferably without using any imports.

\$\endgroup\$
3
  • 7
    \$\begingroup\$ Welcome to CoreReview@SE. When/as you don't tell what to consider better, be prepared for personal preferences. The problem statement is funny in using similar - close to an integral multiple of 42? \$\endgroup\$
    – greybeard
    Feb 2, 2020 at 9:45
  • 10
    \$\begingroup\$ For what it's worth, your solution is simple and obviously correct. I'd prefer it over many of the shorter less-obviously correct solutions below. As a bonus, your solution is easily transcribed into almost any language! \$\endgroup\$
    – Ben
    Feb 3, 2020 at 19:10
  • \$\begingroup\$ grumble grumble The true correct way is using binary operations... \$\endgroup\$ Feb 4, 2020 at 20:32

8 Answers 8

15
\$\begingroup\$

Document your code. In the code.

Code the way you think about the problem - which may very well be as presented in the question.
Leave a code comment where you have unresolved concerns (like
there should be a better way to code this, because a == b features in two conditions of a single elif chain).

def solitary_numbers_product(a, b, c):
    """ return the product of the numbers specified,
        ignoring numbers occurring more than once.
    """
    if a == b:
        return 1 if b == c else c
    if a == c:
        return b
    if b == c:
        return a

    return a * b * c


solve = solitary_numbers_product
# finger exercise in functional coding
# PEP 8 puts imports at module top
from collections import Counter
from functools import reduce


def unique_numbers_product(*numbers):
    """ return the product of the numbers specified,
        ignoring numbers occurring more than once.
    """
    counts = Counter(numbers)
    return reduce(lambda product, factor: product * factor,
                  filter(lambda n: counts[n] == 1, counts), 1)


if __name__ == '__main__':
    # ToDo: develop a decent test
    numbers = tuple(ord(c) - ord('0') for c in '2353332')
    for offset in range(5):
        triple = numbers[offset:offset + 3]
        print(solve(*triple))
        print(unique_numbers_product(*triple))
\$\endgroup\$
8
  • \$\begingroup\$ (Darn. b == c occurs more than once…) \$\endgroup\$
    – greybeard
    Feb 2, 2020 at 11:04
  • 13
    \$\begingroup\$ don't be too afraid of repeating code, if it makes the code more readable. I think your solve would be more readable if the first condition was a == b and b ==c and removing the ternary operator and adding an extra if statement. It would be more symmetrical. \$\endgroup\$
    – Jan Kuiken
    Feb 2, 2020 at 11:34
  • \$\begingroup\$ The "conditional expression" is how I currently think about it (if I didn't immediately generalise to multiple numbers) - if it wasn't, I'd have left a comment more likely than not. I'm not overly concerned about static repetition when I don't see a way to (readably) avoid it - one problem with a == b in the question is that without recognition of common sub-expressions/conditions, it might be evaluated twice at runtime. \$\endgroup\$
    – greybeard
    Feb 2, 2020 at 11:48
  • 1
    \$\begingroup\$ While there is a built-in function sum(), but no product(), the lambda used is obsolete: operator.mul is part of the Python library, and, as of 3.8, math.prod(). \$\endgroup\$
    – greybeard
    Feb 3, 2020 at 7:11
  • \$\begingroup\$ why not tuple(int(c) for c in '2353332') instead of tuple(ord(c) - ord('0') for c in '2353332') \$\endgroup\$ Feb 3, 2020 at 10:25
29
\$\begingroup\$

For what it's worth, I think your solution is clearer than any of the alternatives posted so far. They are shorter in some cases, but I note that you asked for a better solution, not a shorter one. In this case, I don't believe that brevity is an improvement.

I am not a python programmer, but I can understand your solution. It does what it is supposed to do and it is clear what is happening in each step, and what the result will therefore be. A brief comment at the start of the method explaining what it does might be nice, and perhaps a more descriptive name for the method (although not sure what that would be in this case, since the operation seems rather arbitrary). Other than that, unless other constraints are placed on this method, such as performance, I would not change it.

The only alternative I can see that would be equally readable would be to elide the product variable entirely and simply use return statements, i.e.

if a == b == c:
    return 1
elif a == b:
    return c
elif a == c:
    return b
elif b == c:
    return a

return a * b * c

However, I do not think this is better, simply different, and as clear. Note that this solution has more than one exit point, but it is largely a matter of opinion whether that is a bad thing or not, so I will leave that for you to decide.

\$\endgroup\$
1
  • 8
    \$\begingroup\$ Absolutely. The conditional branches in the OP's code match precisely with the wording of the problem, so it's clear that it does the right thing. \$\endgroup\$
    – Doris
    Feb 3, 2020 at 17:57
19
\$\begingroup\$

For greater brevity, you could also solve this puzzle using Python's functional programming tools:

import operator
from collections import Counter
from functools import reduce


def solitary_product(*numbers):
    """
    Return the product of the given numbers, ignoring all numbers
    that repeated anywhere.
    """
    counts = Counter(numbers)
    distinct_numbers = [n for n in numbers if counts[n] == 1]

    return reduce(operator.mul, distinct_numbers, 1)

This has the added benefit of extending your solution to collections of numbers of arbitrary length instead of just sequence of three numbers.

It is worth noting that this method would likely be less efficient for short sequences of numbers due to the added overhead of additional functions calls and constructing a new collection. However, for longer sequences, this method offers the most ideal performance, and will be much more readable than long if-else chains.


Update for Python 3.8:

The math module now includes the prod function, which can compute the product of the elements in any iterable. Using this new feature, we can implement this solution more briefly as

import math
from collections import Counter


def solitary_product(*numbers):
    """
    Return the product of the given numbers, ignoring all numbers
    that repeated anywhere.
    """
    counts = Counter(numbers)
    distinct_numbers = [n for n in numbers if counts[n] == 1]

    return math.prod(distinct_numbers)

Thanks to L.F. for bring this to my attention.

As mentioned in the comments, you could even express this as a one-liner at the expense of readability:

def solitary_product(*numbers):
    return math.prod(n for n, counts in Counter(numbers).items() if counts == 1)
\$\endgroup\$
13
  • 10
    \$\begingroup\$ This is by far the best answer. All the others are ad hoc code that handles exactly three numbers; if next week, the program needs to handle a fourth number, the given algorithms would have to be completely rewritten, and would be very complicated. Handling the general case, as this answer does, is always the right way to do it. \$\endgroup\$ Feb 3, 2020 at 2:05
  • 1
    \$\begingroup\$ Shouldn't it be if len(distinct_numbers) == 0: return 1 and I would do distict_numbers = {number for number, count in Counter(numbers).items() if count ==1} \$\endgroup\$ Feb 3, 2020 at 10:29
  • 7
    \$\begingroup\$ Ray Butterworth I disagree, premature generalisation can be just as bad as premature optimization, and should not be encouraged at the expense of clarity and simplicity. \$\endgroup\$
    – Doris
    Feb 3, 2020 at 11:14
  • 1
    \$\begingroup\$ That test len(distinct_numbers) == 1 seems pointless and wrong. If we pass (2, 2, 3), then distinct_numbers is {3} and we want to return 3, not 1. I think the test should be if not distinct_numbers: instead. Or, just unconditionally insert 1 to the set before invoking reduce or math.prod. Actually, with math.prod(), we can pass an empty iterable, so it's not necessary in that version. Then the function becomes a one-liner: return math.prod({n for n,c in Counter(numbers).items() if count == 1}) \$\endgroup\$ Feb 3, 2020 at 11:26
  • 4
    \$\begingroup\$ @doris, the general case is often simpler and clearer. Code like if a == b: return 1 if b == c else c doesn't make what is happening obvious. With code like that it's not obvious that the algorithm properly handles all cases. And think what it would look like if it had to handle 4 numbers. Code that obviously reflects the algorithm, "remove duplicates, append 1 to the list, return the product", is much easier for anyone to understand, debug, or modify, and gives much more confidence that it actually does what it claims to do. In 90+% of code, efficiency doesn't matter, so add that later. \$\endgroup\$ Feb 3, 2020 at 14:33
9
\$\begingroup\$

There is no need for the initial product == 0

And you can simplify the elif tree with early returns

def solve2(a, b, c):
    if a == b == c:
        return 1
    if a == b:
        return c
    if a == c:
        return b
    if b == c:
        return a
    return a * b * c

This makes the intent and logic very clear.

You can use the fact that in python, True and False are used as 1 and 0 in calculations:

def my_product(a, b, c):
    return (
        a ** (a not in {b, c})
        * b ** (b not in {a, c})
        * c ** (c not in {a, b})
    )

or

def my_product2(a, b, c):
    return (
        a ** (a != b and a != c)
        * b ** (b != a and b != c)
        * c ** (a != c and b != c)
    )

or using the new python 3.8 math.prod

import math


def my_product_math(a, b, c):
    return math.prod(
        (
            a if a not in {b, c} else 1,
            b if b not in {a, c} else 1,
            c if c not in {a, b} else 1,
        )
    )

Then you need a few test cases:

test_cases = {
    (2, 3, 5): 30,
    (3, 5, 3): 5,
    (5, 3, 3): 5,
    (3, 3, 3): 1,
    (3, 3, 2): 2,
}

and you evaluate them like this:

[my_product(a,b,c) == result for (a,b,c), result in test_cases.items()]

You can even time this:

import timeit
timeit.timeit(
    "[func(a,b,c) == result for (a,b,c), result in test_cases.items()]",
    globals={"func": my_product, "test_cases": test_cases},
)

and the all together behind a main guard:

if __name__ == "__main__":
    test_cases = {
        (2, 3, 5): 30,
        (3, 5, 3): 5,
        (5, 3, 3): 5,
        (3, 3, 3): 1,
        (3, 3, 2): 2,
    }

    methods = [
        solve,
        solve2,
        my_product,
        my_product_math,
        solitary_product,
        solitary_numbers_product,
        solve_graipher,
        solve_kuiken,
        solve_kuiken_without_lambda,
        my_product2,
    ]

    for method in methods:
        result = all(
            [
                method(a, b, c) == result
                for (a, b, c), result in test_cases.items()
            ]
        )
        time = timeit.timeit(
            "[func(a,b,c) == result for (a,b,c), result in test_cases.items()]",
            globals={"func": method, "test_cases": test_cases},
        )
        print(f"{method.__name__}: {result} - {time}")

Which shows that in terms of speed, your method is one of the fastest

solve: True - 2.324101332999817
solve2: True - 2.386756923000121
my_product: True - 6.072235077000187
my_product_math: True - 5.299641845999986
solitary_product: True - 19.69770133299994
solitary_numbers_product: True - 2.4141538469998522
solve_graipher: True - 4.152514871999756
solve_kuiken: True - 7.715469948999726
solve_kuiken_without_lambda: True - 5.158195282000179
my_product2: True - 5.210837743999946

So I would go with the simplification of your original algorithm

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I think using elif is better even when you are returning early, since it makes it more obvious that only one of these branches will execute (without the reader having to check that every block has a return). I would even recommend using else for the last case. \$\endgroup\$
    – kaya3
    Feb 4, 2020 at 14:24
  • \$\begingroup\$ That's a matter of preference \$\endgroup\$ Feb 4, 2020 at 14:25
7
\$\begingroup\$

You can eliminate one branch by sorting the inputs:

def solve(a, b, c):
    if a == b == c:
        return 1
    a, b, c = sorted([a, b, c])
    if a == b:
        return c
    elif b == c:
        return a
    return a * b * c

This makes it a bit shorter. I also like the explicit structure of this code, it is very readable and immediately obvious what happens.

Having immediate returns makes the code also easier to read IMO, although some design philosophies prefer having only a single return per function, as you currently have.

In order to make this even clearer, you should add a docstring describing what the function does. Unless required by the challenge, solve is also not a good name for this function, because it does not tell you anything about what the function actually does.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thinking out-of-the-box! (getting an TypeError: 'int' object is not iterable if not bracketing the literal argument to sorted().) You can shave off another comparison putting return 1 after the sorted(): return 1 if a == c else c if a == b else a if b == c else a * b * c;. (Ever managed to un-see something?) \$\endgroup\$
    – greybeard
    Feb 2, 2020 at 16:07
  • \$\begingroup\$ @greybeard: Fixed. However, I'm not a great fan of ternary expressions, unless it is in the most trivial cases, which this most certainly is not. But sure, that would also work :) \$\endgroup\$
    – Graipher
    Feb 2, 2020 at 16:09
  • \$\begingroup\$ @Graipher: I'm not sure about the sorted , my brain has to work harder (and maybe the Python interpreter also) \$\endgroup\$
    – Jan Kuiken
    Feb 2, 2020 at 19:59
2
\$\begingroup\$

Yet another solution, using a dictionary for pattern matching. I do not know how it performs in comparison with previous solutions, but dictionaries in Python are known to be pretty efficient. Note: the keys with two True's are skipped because they cannot occur.

SOLVE_TABLE = { ( False , False , False ) : lambda a,b,c: a * b * c,
                ( False , False , True  ) : lambda a,b,c: b,
                ( False , True  , False ) : lambda a,b,c: a,
                ( True  , False , False ) : lambda a,b,c: c,
                ( True  , True  , True  ) : lambda a,b,c: 1  }

def solve(a,b,c):
    return  SOLVE_TABLE[(a==b, b==c, c==a)](a,b,c)
\$\endgroup\$
5
  • \$\begingroup\$ Nice substitute for the switch statement I might have used in a language providing one, and an alternative to encoding the comparison results and using that as an index. (Nice in only evaluating the product when selected (unless I'm mistaken).) \$\endgroup\$
    – greybeard
    Feb 2, 2020 at 22:26
  • \$\begingroup\$ I see no use to the lambda if you call it immediately. The advantage of only evaluating it when selected is lost by having to construct 6 anonymous functions \$\endgroup\$ Feb 3, 2020 at 10:17
  • \$\begingroup\$ @MaartenFabré my idea with the lambda's was to make the dictionary constant, and it would be evaluated only once. I don't think it does, when I define the dict outside the function, as a 'constant', I see a speed increase. \$\endgroup\$
    – Jan Kuiken
    Feb 3, 2020 at 15:23
  • \$\begingroup\$ Then I can see the point. To make this more clear, you can move this dictionary definition outside the function body. \$\endgroup\$ Feb 3, 2020 at 15:25
  • 3
    \$\begingroup\$ -1. I dislike this so much.. it’s horrible... I would damn the guy who writes this at work. somewhat funny because “thinking outside of the box” but other than that more of a perfect example of how not to write code IMO. I don’t want to see ‘funny code’ at work. Not to the point (what is the meaning of the last false in the fourth row again?), not readable, error prone (switch one of those booleans), implicit stuff (skipping two trues because blablabla) and last but not least: it scales horribly (4 numbers? 5? 6? Have fun with that table and skipping all the cases that can’t occur).. \$\endgroup\$ Feb 4, 2020 at 5:06
2
\$\begingroup\$

Other solutions seem really clumsy and very hard to read, here is a nice simple way to do this:

from collections import Counter

def solve(a, b, c):
    unique = {a, b, c}
    if len(unique) == 1:
        return 1
    if len(unique) == 3:
        return a * b * c
    return Counter((a, b, c)).most_common()[-1][0]
\$\endgroup\$
0
0
\$\begingroup\$

Here's a solution using dictionaries and mapping (and no additional imports):

def solve(a, b, c):
  l = [a, b, c]
  c = lambda x: l.count(x) > 1
  z = [c(v) for v in l] # Replacing the import of Counter

  {
      (True, True): 1, 
      (False, False): l[0]*l[1]*l[2]
  }.get(map(lambda b: b(z), [all, any]), l[z.index(False)])

So, if all elements are equal, the resulting map will be True for all and True for any, if none are equal, it will be False for both. If the get() fails, return the one that has the False index in z list (the list that states, for every element, if there is more than one element of that).

\$\endgroup\$
1
  • 1
    \$\begingroup\$ There is no need for the lambda c. 1 letter variable names are very unclear and this method is rather convoluted. numbers= (a,b,c); lonely= [(numbers.count(number) <1)for number in numbers]; return { (True, True): a*b*c, (False, False):1 }.get((any(lonely), all(lonely)), numbers[lonely.index(True)]) is more clear, but still very convoluted \$\endgroup\$ Feb 4, 2020 at 8:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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