4
\$\begingroup\$

I wrote this program to print how many times each number occurs in a list:

class deleteduplicate:

    def __init__(self):
        a=[]
        num=int(input("Enter the number of elements"))
        for i in range(0,num):
            try:
                a.append(int(input("Enter the number {}".format(i))))
            except ValueError:
                print ("Wrong Input")
                i=i-1

        flagarray=[]
        countarray=[]
        j=0;
        for c in a:
            flagarray.append(False)
            countarray.append(1)

        for c in a:
            count=1
            flag=False
            ca=[]
            for i in range(j,len(a)):

                if a[i] == c and flagarray[i]==False:


                    flag=True
                    count=count+1
                    flagarray[i]=True
                    ca.append(i)
            if  len(ca)>0:
                print(str(c)+"ocuurs"+str(len(ca))+"times")

            j=j+1

deleteduplicate()
\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to CodeReview. It looks like you are new to python. Feel free to read How to Ask if you would like to know more about asking questions. A general Python resource that many here use is PEP-8. \$\endgroup\$ – Gloweye Oct 16 at 7:29
5
\$\begingroup\$

Welcome to code review.

Style

Check PEP0008 the official Python style guide for maintaining code within Python acceptable standards.

  • Class names: Apart from the fact that this is a bad use of classes, class names should be UpperCaseCamelCase which implies that since this delete_duplicates is a class, then the name should be DeleteDuplicates.
  • Method/function/variable names: should be lowercase and words separated by underscores ex: flagarray=[] is flag_array = []
  • Space around operators: a space should be left on both sides of a binary operator: a=[] is a = [] and flagarray=[] is flag_array = []
  • Descriptive variable names: names should reflect the objects they represent and not names like j, c, i which are confusing because j could be a variable, a string, a number ...
  • f-strings: (Python 3.6 +) are better used for combining strings and variables in a single statement. ex: print(str(c)+"ocuurs"+str(len(ca))+"times") can be print(f'{c!s} occurs {len(ca)} times')

Code

  • Functions: A function is a block of code which only runs when it is called. You can pass data, known as parameters, into a function. A function can return data as a result. Since this a bad use of classes because a class is usually used for an object with multi-attributes and multi-methods and this is not the case here, you might use a regular function.
  • Augmented assignment: Python supports augmented assignments ex: i=i-1 is i -= 1, count=count+1 is count += 1 ...
  • Semicolons: are for combining multiple short statements on the same line, this j=0; (line 15) is an invalid use of semicolons.
  • Comparison to True and False: is usually done using if something: and if not something_else: ex: flagarray[i]==False: is if not flag_array[i]:. Non-empty sequences and non-zero variables evaluate to True ex: if len(ca)>0: is if ca:
  • Inefficiency: If you need to print duplicates of a list you shouldn't be entering each of the list members manually (what if you have a list of size 10,000,000 items? will you be manually entering one by one?)
  • Counter() You could use counter dict from the collections library for counting list members

An improved version:

If you want to get duplicates and print them:

from collections import Counter


def get_duplicates(numbers: list):
    """Return list duplicates."""
    num_duplicates = Counter(numbers)
    return {item for item in num_duplicates if num_duplicates[item] > 1}


if __name__ == '__main__':
    list_of_duplicates = [1, 2, 2, 3, 4, 3, 5, 2, 7, 1, ]
    print(get_duplicates(list_of_duplicates))

If you want to delete duplicates then use a set

print(set(list_of_duplicates))

output: {1, 2, 3, 4, 5, 7}

\$\endgroup\$
  • \$\begingroup\$ thanks bullseye.This is great \$\endgroup\$ – gokul Oct 16 at 8:57
  • \$\begingroup\$ bullseye .Actually i didnt understand what is happening in this line. return {item for item in numbers if Counter(numbers)[item] > 1} can you please explain this to me \$\endgroup\$ – gokul Oct 16 at 9:00
  • \$\begingroup\$ @gokul Thank you for accepting my answer, you should be using the upvote ^ as well however if you still need answers then I suggest you unaccept my answer and use the upvote ^ for now and decide later which answer is the most helpful. \$\endgroup\$ – bullseye Oct 16 at 9:05
  • \$\begingroup\$ @gokul {} are set delimiters, a set does not contain duplicates(each set member will appear once) if you need to understand how sets work, here's a link w3schools.com/python/python_sets.asp and if you do not understand the syntax, this is called comprehension syntax and here's another link for comprehensions pythonforbeginners.com/basics/list-comprehensions-in-python \$\endgroup\$ – bullseye Oct 16 at 9:08
  • 1
    \$\begingroup\$ @bullseye: I just added my own answer. \$\endgroup\$ – Graipher Oct 16 at 16:28
3
\$\begingroup\$

First off, this really shouldn't be a class, but a function.

You should have a look at PEP 8, which is the closest thing to a Python style guide.

For clarity, you should also split it up so that you have the processing separate from the input (and it's validation.) The general structure of your program might look a bit like this:

def delete_duplicates(mylist):
    # Do stuff

def get_input():
    # Do stuff

if __name__ == "__main__":
    delete_duplicates(get_input())

We call that last bit a guard - it basically ensures that the script only executes when it is run as a script, but not when we import this into another script.

Since it looks to me like it's about the process of removing duplicates from a list and not about the input here, I'll skip the input bits for now.

From what your code looks to output, you seem to be counting the amount of times something occurs in a list. Python has builtin tools for that, like the count() method. It works like this:

num_occur = mylist.count(2)  # Counts how often the int 2 is in mylist.

What you don't seem to do is actually remove the duplicates from that list, while your class name does claim to do so. You could of course use list.remove(elem) to remove a single occurrence of elem from your list. Another way to do this may be:

new_list = list(set(old_list))

This removes all duplicates by transforming your list into a set, and then back in a list. Sets cannot have duplicates, so this technique removes them all. However, if the order is preserved you can count it as a happy accident. While it'll be common if you do it with a list like [1, 2, 3], when things get more complex they tend to get shuffled.

You also seem to have a habit of calling lists arrays. They're really not, even if they look and sometimes seem to act like them on the surface. Arrays tend to be much lower level structures than lists, and it may be useful to read up more on python lists.

\$\endgroup\$
  • \$\begingroup\$ Thanks Gloweye for your review \$\endgroup\$ – gokul Oct 16 at 8:57
  • \$\begingroup\$ @gokul please upvote answers you found helpful using the (^ sign on the left). \$\endgroup\$ – bullseye Oct 16 at 9:02
  • \$\begingroup\$ @AlexV yeah, I forgot about that \$\endgroup\$ – bullseye Oct 16 at 9:10
  • \$\begingroup\$ @AlexV They do now :-) \$\endgroup\$ – Graipher Oct 16 at 11:55
1
\$\begingroup\$

Since you are a beginner, this is a good opportunity to learn about Python's standard library, its data structures and best practices.

Before starting, you should always separate user in-/output and the actual calculations, as recommended in other answers. I'm going to assume that from now on.

It is also not quite clear how to interpret your question. Should your code just note all duplicate items or should it remove them?


Let's first look at the second possibility. One way to find out how often each element appears in a list is to use list.count. With this we could do:

def make_unique(x):
    return [v for v in x if x.count(v) == 1]

This uses a list comprehension that works similar to a for loop but is more compact and slightly faster. However, the algorithm is not perfect, because list.count checks the whole list every time it is called, so this is \$\mathcal{O}(n^2)\$.

If we don't care about the order, then we can just use a set, which is a lot faster since it only iterates through our list once, which makes it \$\mathcal{O}(n)\$:

def make_unique(x):
    return list(set(x))

Another alternative if we do want to keep the order is to use the itertools recipe unique_everseen:

from itertools import filterfalse

def unique_everseen(iterable, key=None):
    "List unique elements, preserving order. Remember all elements ever seen."
    # unique_everseen('AAAABBBCCDAABBB') --> A B C D
    # unique_everseen('ABBCcAD', str.lower) --> A B C D
    seen = set()
    seen_add = seen.add
    if key is None:
        for element in filterfalse(seen.__contains__, iterable):
            seen_add(element)
            yield element
    else:
        for element in iterable:
            k = key(element)
            if k not in seen:
                seen_add(k)
                yield element

It is a bit more complicated, because it also allows to define a key function which is used to determine if an object has been seen before. Internally it keeps a set, but it yields elements in order. It also iterates over the list exactly once.


If you want to get all elements that are repeated, you could also use list.count:

def get_duplicates(x):
    return [v for v in x if x.count(v) > 1]

However, storing seen elements is a better solution. To also keep track of how often you have seen them, you can store them in a dict:

def get_duplicates(x):
    dupes = {}
    for v in x:
        if v in dupes:
            dupes[v] += 1
        else:
            dupes[v] = 1
    for v in x:
        if dupes[v] > 1:
            yield v, dupes[v]
            dupes[v] = 0

This needs to make two passes over the list, just like your code. One to count all elements (you just set a flag), and one to yield them in order, together with how often they appeared. By setting the value to 0 afterwards, each value appears only once.

That first loop can be shortened a bit by using a collections.defaultdict, so we don't need to concern ourselves with the special case that a value is not yet in the dictionary:

from collections import defaultdict

def get_duplicates(x):
    dupes = defaultdict(int)
    for v in x:
        dupes[v] += 1
    for v in x:
        if dupes[v] > 1:
            yield v, dupes[v]
            dupes[v] = 0

This uses the fact that an int is by default 0. However, we still need two passes. So, instead we can use a collections.Counter, which was made for exactly this situation. You want to count how often each item appears.

from collections import Counter

def get_duplicates(x):
    dupes = Counter(x)
    for v, c in dupes.items():
        if c > 1:
            yield v, c

But this still iterates twice, once over the whole list to build the Counter, and once over all unique values in it. To avoid that we can use the method Counter.most_common, which returns the items from the counter, sorted from most common to least. Together with itertools.takewhile, which keeps on taking items until the condition is false:

from collections import Counter
from itertools import takewhile

def get_duplicates(x):
    return ((v, c)
            for v, c in takewhile(lambda t: t[1] > 1, Counter(x).most_common()))

This returns a generator (just like all functions with a yield are). You need to iterate over it to get all elements, either with:

x = [1, 2, 2, 4, 2, 2, 6, 7, 2, 2, 5, 7]
for v, c in get_duplicates(x):
    print(f"{v} occurred {c} times")

You can also use list to exhaust the generator:

dupes = list(get_duplicates(x))

This function is \$\mathcal{O}(n)\$ for the Counter and \$\mathcal{O}(k)\$ with \$k \le \frac{n}{2}\$ the number of elements that appear more than once. In total that gives you \$\mathcal{O}(n)\$ at worst.


Now let's get to the first part of your function, asking the user for input. For this it makes sense to make a function that keeps on asking the user until they supplied a valid input. While your function guards against wrong input for the elements of the list, the user can enter anything for the length. Including "foobar", which will crash the program.

def ask_user(message, type_=None):
    while True:
        user_input = input(message)
        if type_ is None:
            return user_input
        try:
            return type_(user_input)
        except ValueError:
            print("Wrong Input")

if __name__ == "__main__":
    n = ask_user("Enter the number of elements", int)
    x = [ask_user(f"Enter the number {i}", int) for i in range(n)]

Note that I used type_ instead of type, because that is a built-in function. Using a trailing underscore in that case is a common workaround.

\$\endgroup\$
  • 1
    \$\begingroup\$ thankyou so much Graipher \$\endgroup\$ – gokul Oct 17 at 7:18

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.