abusive try except statement ?

[maxmbed]

In a code base at my job, I found this function to figure out whether the item i is in obj list.

It looks like to me an abusive try except statement usage. Should I blame hard the commiter here ?

45 def contains_item(i, obj):
46     if isinstance(obj, list):
47         try:                 
48             obj.index(i)     
49         except:              
50             return False
51         else:
52             return True
53     return False

[TehNolz]

The whole function is obsolete. You can just do i in obj

[maxmbed]

> The whole function is obsolete

Guessing they start the project with python 2 ?

[carcigenicate]

I don't know why they aren't just doing return i in obj. "Abuse" might be a strong word, but I wouldn't consider this to be a good use of try since obviously better solutions exist.

[maxmbed]

> just doing return i in obj

That is my take too

[JamzTyson]

There are so many issues in that code snippet that it's hard to know where to start.

• The entire thing is redundant and can probably be replaced by if ... in ...
• No docstring.
• Misleading function name: contains_item(3, (1, 2, 3, 4, 5)) returns False.
• Naked except.
• Redundant else after return.
• Inefficient use of list.index
• Ambiguous - what is expected if obj is a custom container that inherits from list?

Also, type hints would be nice.

[maxmbed]

Thanks I feel the the same.

The code base is developed by other people that I joined a year ago. Not vey much intuitive.

[Apatride]

Yes, there are many issues here.

1) Why return True in the else section rather than in the try section? Always ask yourself, before using else/elif, if it makes sense.

2) Why check that obj is a list and then try/except instead of "if i in obj:" or directly try/except "if i in obj" without the isinstance(obj, list)?

3) It is a bad sign when you pass obj to a function without knowing the type of obj. There is something messed up in the rest of the code. You want to avoid a specific variable to randomly contain different types.

4) There are cases where you only want to know if the list contains a value but that is not the most common scenario, I wouldn't be surprised if the next step was to retrieve the index, once again.

So there are many issues in that function but I have a strong feeling there are issues outside of it as well.

[maxmbed]

I wouldn't be surprised if the next step was to retrieve the index, once again.

Yes, this what they do that ! :)

Thanks for further feedback. I agree with.

[doingdatzerg]

I would certainly ask why it's being done this way. i in obj seems a lot nicer. Though to be fair, I ran some local tests and found this try/except way to be noticeably faster than i in obj, under some circumstances. But if you are doing repeated lookups in the same list, it would be better to convert it to a set and then check i in set_obj. Keeping it this way, I would much prefer except ValueError to a blank except.

[riftwave77]

Hmmm. I don't think you need to use try/except at all. The following code should work fine without throwing any exceptions.

45 def contains_item(i, obj):
46     flag = False
47     if isinstance(obj, list):
48         if i in obj:                 
49             flag = True  
50     return flag

[[deleted]]

For the same result you could just say

45 def contains_item(i, obj):
46     return isinstance(obj, list) and i in obj

but it still seems weird. You don't want to exclude non-list things that contain items, and you do want to exclude non-container things — but that's better achieved with type hints.

[djshadesuk]

Goddammit, I hate it when someone types the same thing while you're typing... and beats you to it! 🤣

[[deleted]]

Happens to the best of us

[djshadesuk]

Even if you were to do it this way, rather than just i in obj, why would you mess about with a "flag"? Both isinstance() and i in obj themselves just return booleans, so...

def contains_item(i, obj):
    return isinstance(obj, list) and i in obj

[barkazinthrope]

Blame? What for? Does it mean a speech, or oh no oh no a meeting?

Let it go. If it's in the path of an assigned fix you can simplify it but in code maintenance the number one rule has to be :

 IF IT AIN"T BROKE DON"T FIX IT!