2
\$\begingroup\$

Given an array of ints as input:

  • non_unique([1, 2, 3, 1, 3]) returns [1, 3, 1, 3]
  • non_unique([1, 2, 3, 4, 5]) returns []
  • non_unique([5, 5, 5, 5, 5]) returns [5,5,5,5,5]
  • non_unique([10, 9, 10, 10, 9, 8]) returns [10, 9, 10, 10, 9]

The code itself:

def checkio(originalList: list) -> list:

    checked_list = []
    result_list = []

    for i, number in enumerate(originalList):
        if number in checked_list:
            result_list.append(number)
        else:
            if (len(originalList) - 1) - i > 0:
                if number in originalList[i+1:]:
                    result_list.append(number)
        checked_list.append(number)

    return result_list

How can be the code improved?

\$\endgroup\$
4
\$\begingroup\$

One thing to keep in mind is your variable checked_list, which serves almost no purpose in the function, as you could do without the else block.

The simplest thing I can think of would be just to shorten the function down to one list comprehension, instead of appending to two lists.

Revised code:

def checkio(original_list: list) -> list:
    return [x for x in original_list if original_list.count(x) > 1]

When running this function, these are the outputs:

checkio([1, 2, 3, 1, 3])  # Returns [1, 3, 1, 3]
checkio([1, 2, 3, 4, 5])  # Returns []
checkio([5, 5, 5, 5, 5])  # Returns [5,5,5,5,5]
checkio([10, 9, 10, 10, 9, 8])  # Returns [10, 9, 10, 10, 9]
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Code Review

Keeping your current structure, I would modify your solution like this:

def checkio(originalList: list) -> list:

    result_list = []

    for i, number in enumerate(originalList):
        if number in originalList[:i] or number in originalList[i + 1:]:
            result_list.append(number)

    return result_list

checkio([1, 2, 3, 1, 3])         # Returns [1, 3, 1, 3] 
checkio([1, 2, 3, 4, 5])         # Returns []
checkio([5, 5, 5, 5, 5])         # Returns [5, 5, 5, 5, 5]
checkio([10, 9, 10, 10, 9, 8])   # Returns [10, 9, 10, 10, 9]

The if condition now checks if the element in question appears anywhere else in the list. If so, the element is saved as it is non-unique. Compared to your solution, this function combines the if and else conditions into one check. It also removes checked_list.

To do this task in the most concise way, I would use list comprehension and the Counter subclass from the collections module, as shown below.

Another Approach

from collections import Counter

def checkio(originalList: list) -> list:
    counter = Counter(originalList)                       # Create "Counter" object
    return([x for x in originalList if counter[x] > 1])   # Return only non-unique elements

checkio([1, 2, 3, 1, 3])         # Returns [1, 3, 1, 3] 
checkio([1, 2, 3, 4, 5])         # Returns []
checkio([5, 5, 5, 5, 5])         # Returns [5, 5, 5, 5, 5]
checkio([10, 9, 10, 10, 9, 8])   # Returns [10, 9, 10, 10, 9]

counter counts the occurrences of each element in the list and returns a dictionary. Here's an example:

Counter([10, 9, 10, 10, 9, 8])
# Counter({10: 3, 9: 2, 8: 1})

After creating this object, the function uses list comprehension and only adds elements that appear more than once, ie non-unique elements.

This answer is similar to snarp's. However, for large lists this version will be faster as the Counter object is computed only once (instead of calling original_list.count(x) for every element).

A bit more about the collections module: it has specialized data types with interesting advantages over standard Python objects such as dict, list, set, etc. Here is a link to the documentation.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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