2
\$\begingroup\$

Recently, I got a code test from NAB. Please take a look at this one and give me your idea.

Find the max value of an array of N integers.

E.g. [ 1, -6, 2, 0, 1011, -355]

Require: the max value is a multiple of 3 and the code focuses on the correctness, not performance.

def solution(A):
    try:
        if not isinstance(A, list):
            raise TypeError
        max_value = max([x for x in A if x % 3 ==0])
        return max_value
    except Exception as e:
         print(f"{A} is not a list")
         print(e)

So does my code fulfill the requirements or Did I miss any edge case?

P/s: They announced that was fail and only got 22/100 pts.

\$\endgroup\$
3
  • 5
    \$\begingroup\$ If your code fails to pass the tests, then we presume that it doesn't work correctly as intended, and is therefore not yet eligible for a code review. However, your code is simple, and it's hard to see how it could possibly fail. Is there any more detail about the specification? For example, what exactly are you supposed to do if no member of the array is a multiple of 3? \$\endgroup\$ Commented Apr 21, 2022 at 14:56
  • 3
    \$\begingroup\$ @200_success Well, they did also post it on stackoverflow. \$\endgroup\$ Commented Apr 21, 2022 at 18:38
  • 2
    \$\begingroup\$ "Require: the max value is a multiple of 3 and the code focuses on the correctness, not performance." Is it possible this means you should only return the max value if it is divisible by 3 vs. return the maximum element that is multiple of 3? \$\endgroup\$
    – dlev
    Commented Apr 21, 2022 at 22:48

2 Answers 2

4
\$\begingroup\$

Functions like this should not print. They should return an answer as data or, if that is impossible, either raise an exception or return None.

The function should not be restricted to lists. It can easily handle any iterable. But under most circumstances, I would not bother enforcing a type check like this, because it tends to spawn additional annoying questions: if the iterable is type checked, must we also enforce that its values are numeric (or more precisely, that the values are comparable and support the % operator)? Ultimately, users of a function must understand its purpose. Docstrings and other forms of communication are often more helpful than hyper-diligent checking.

The built-in max function already has a way to deal with empty sequences. Namely, the default argument.

def solution(xs):
    return max((x for x in xs if x % 3 == 0), default = None)
\$\endgroup\$
0
\$\begingroup\$

The only option why your score is so low is that maybe they want you to avoid using Python built-in functions and craft your own max by yourself.

The classic way is doing it with a cycle over all the elements and check the max placeholder variable for each element divisible by 3.

Another way could be using recursion over the solution function, like this:

def solution(A):
    # base of recursion
    if len(A) == 0:                                         
        return None
    if len(A) == 1:                       
        return A[0] if A[0] % 3 == 0 else None

    # recursive steps
    if A[0] % 3 > 0:                                     # 1st elem is not divisible by 3
        return solution(A[1:])
    if A[1] % 3 > 0:                                     # 2nd elem is not divisible by 3
        return solution([A[0]]+A[2:])
    return solution(A[0]+A[2:] if A[0]>A[1] else A[1:])  # compare 1st and 2nd

tests = [
    ([]                       , None, 'empty list'            ), 
    ([4]                      , None, 'no nums divisible by 3'), 
    ([3]                      , 3   , 'one num divisible by 3'),
    ([4, -9]                  , -9  , 'two values'            ),
    ([1, -6, 2, 0, 1011, -355], 1011, 'input sample'          ),
]

for test in tests:
    print(f'CASE {test[2]}:', 'PASSED' if solution(test[0])==test[1] else 'NOT PASSED')

I wouldn't check for input type, as the problem statement assumes that the input is an array of integers.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Python's recursion limit usually 1000, so this approach won't scale very far. It also does a lot of data-copying, via the slicing, again raising significant scaling problems for anything other than tiny sequences. \$\endgroup\$
    – FMc
    Commented Apr 21, 2022 at 15:20
  • \$\begingroup\$ Can't agree more on scalability @FMc, this solution takes into account my beginning assumption: craft your own max by yourself, then two possibilities are laid, the first with the cycle (code not written) and the second one. As long as the code focuses on the correctness, not performance according to the problem statement, I've given both these solutions for good. \$\endgroup\$
    – lemon
    Commented Apr 21, 2022 at 15:53

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