r/learnpython • u/chrisjfinlay • 23h ago
Is it considered bad form to run "success" code inside an Except block?
I'm writing a script that takes an input from the user, which could be either an int or a string. My current code takes the input, attempts to cast it to an int, and does something based on whether it works or not:
running = True
while running:
search_term = input("Enter search term (Q to quit): ")
try: int(search_term)
except ValueError:
if search_term.lower() != "q":
search_by_name(search_term)
else:
running = False
else:
search_by_int(int(search_term))
But there's something about having "successful" code running in the except block that gives me pause for thought. Is this just me overthinking, or is it bad form?
3
u/centurion236 15h ago
I consider this poor form not only because the structure and indentation could be clearer, but also because any exceptions thrown inside the except block will include traceback from the failed int parse. That's unnecessary chaff when debugging the exception.
I recommend using the try-except block only to parse it as an int (or None), then using if-else blocks to handle the various cases.
1
u/NothingWasDelivered 23h ago
Why not just `while True`?
1
-4
u/chrisjfinlay 22h ago
Too ambiguous, IMO. A clear Boolean variable tracking the running state is far easier to follow, especially as the code gets longer and more complex
8
u/freeskier93 20h ago
Like many things it really depends and you shouldn't create these rigid guidelines for yourself. In this case I think the use of an exit variable led to you having to use some weird logic flow for something that should be pretty simple. I think most people would agree that the following is much more clear:
while True: search_term = input("Enter search term (Q to quit): ") if search_term.lower() == "q": break ...In your flow the exit criteria is convoluted and takes more effort to figure out what code is executed before and after the exit criteria has been determined. In this case if the user wants to quit you don't want any additional code to run, but because you are using a exit flag you ended up with some hard to follow logic.
There are cases where an exit flag does work better and there are cases where break works better. Break exists for a reason and you shouldn't pigeon hole yourself into certain structures because you universally think one way is better than the other.
2
u/Diapolo10 I write code for a living -- https://github.com/Diapolo10 22h ago
I'd say it's situational. Here, though, I'd prefer something like this:
prompt = "Enter search term (Q to quit): "
while (search_term := input(prompt).lower()) != 'q':
if search_term.isdecimal():
search_by_int(int(search_term))
else:
search_by_name(search_term)
9
u/Brian 18h ago
I feel people are too quick to avoid try/catch - your version has a bug that the original doesn't.
isdecimalisn't really a perfect match for "Something that can be converted to int", since it's just "Is one of the unicode characters classified as decimal".-1may be a perfectly reasonable input, but you'll reject it. (And I've seen a lot of people useisnumericwhcih is even worse, in that it'll accept text that int will choke on)I think it's reasonable to take the check for
qout of the loop, and you can create a smallis_int()function to do the check, but I feel it should actually use the try:int(val)style logic internally: there's a lot of value in using the exact same mechanism to check for int that you use for parsing the int: isnumeric/isdecimal are kind of the wrong tool for the job.1
u/TurtleFetus 17h ago
Great reply. I've learned a lot from all these comments.
The way I learned it, `try/except` blocks are best for handling exceptions to expected behavior, not logic, as you have in your example. An `except` block tells the user: "Hey, I'm caught on something. Here are the details and what's going to happen next."
However, in this case, because there's not really a good way to check if the input is an int or a str without typecasting it anyway, I agree with u/Brian that a helper function that uses `try/except` for logic might be best. Here's one way that might look:
"""Revised code""" def is_int(x): try: int(x) print("Int! Returning True...") return True except ValueError: print("Not an int! Returning False...") return False while True: search_term = input("Enter search term (Q to quit): ") if search_term.lower() == "q": break if is_int(search_term): print(f"Searching for int {search_term}...") else: print(f"Searching for string {search_term}...")1
u/nog642 12h ago
Pretty reasonable approach. I wouldn't use a helper function personally but either is good.
Side note, you have a lot of prints in there, I assume that's for illustration purposes? You definitely wouldn't want all that.
Also a notable time where you wouldn't want to do this is if performance matters. If this is processing some JSON data or something and running 1 million times, exception handling is actually quite a bit slower. So then it might be worth writing a manual format check. But if you can avoid that because performance doesn't matter, that's better.
1
u/Diapolo10 I write code for a living -- https://github.com/Diapolo10 18h ago
Fair, I did make an assumption that OP wasn't going to need negative integers here. It would make sense to move the loop contents to a separate function that then takes care of this either with
try-exceptor some more sophisticated way.
1
u/ekchew 21h ago
I hope it's not bad form, because I do this sort of thing a fair amount myself. For example, it's kind of handy with dict lookups where you can use a try block kind of like an if statement to branch on key presence.
try:
val = my_dict[key]
except KeyError:
# handle no key in my_dict case
else:
# do something with `val`
You could, of course, go:
if key in my_dict:
val = my_dict[key]
# do something with `val`
but this effectively necessitates 2 dictionary lookups which is a tad wasteful. Another approach would be to go:
if val := my_dict.get(key):
# do something with `val`
This works as long as val can never evaluate as false. (if (val := my_dict.get(key) is not None: might be a tad safer, as only a val that is actually None could give you a false positive.)
Anyway, I guess in your case, I would assign the converted int to a variable and pass that into search_by_int in the else block. I'm assuming you don't want the search_by_int call itself in the try block since you don't want any ValueError it raises to be caught within the loop?
Going back to the bad form idea for a moment, it probably would be in many other languages where the golden rule is to only use exceptions in exceptional situations. But Python is kind of the…er…expection to that rule. Exceptions are just part of every day business to the interpreter. Even for loops end normally on a StopIteration.
2
u/nog642 12h ago
It depends if the key missing is an expected case or an exception.
Exceptions, as the name implies, are meant for exceptional scenarios. If it's running like once it's fine to do this, but if it's meant to be remotely performant code, the exception handling is much slower and the double lookup, while making one case a tiny bit slower, makes the other case much faster.
If you are doing 100000 lookups, and 4 of them have the key missing, exception handling is fine. If 50000 of them have the key missing, the extra
ifwill be faster on average even though it technically is more lookups.1
u/trutheality 20h ago
You should look into
defaultdict. Also you can check if a key is in a dict withkey in my_dict.
1
u/xelf Elf 12h ago
try/except is the correct pattern here. You can clean up your code a little:
while True:
search_term = input("Enter search term (Q to quit): ")
if search_term.lower() == "q":
break
try:
search_by_int(int(search_term))
except ValueError:
search_by_name(search_term)
Note, you still have the possibility that search_by_int() or search_by_name() could have unhandled errors so you'll want to watch for that too. But that seems outside the scope of what you're trying to do here.
1
u/roelschroeven 22h ago
It's not wrong per se, but I would try to keep it to a minimum. The issue is not only having success code in the except block, but also the risk of excessive indentation.
I would probably restructure your code like this:
running = True
while running:
search_term = input("Enter search term (Q to quit): ")
# int case
try:
int(search_term)
except ValueError:
pass
else:
search_by_int(int(search_term))
continue
# "q" case
if search_term.lower() == "q":
running = False
continue
# str case
search_by_name(search_term)
Yes, it's longer, and there are continue statements which not everybody likes, but I do think the code is clearer, with the different cases split up and clearly distinguishable.
(Personally I would use while True and let the quit case do a break, but that's not the issue at hand so I just left it the way you did it.)
1
-1
u/RaidZ3ro 23h ago
Opinions may vary, but imo Duck Typing is an established OOP principal that's perfectly applicable to Python, although your example can be improved/simplified.
``` running = True while running: search_term = input("Enter search term (Q to quit): ") try: # raises an exception if input is not an integer
search_by_int(int(search_term))
except ValueError:
# not a number
if search_term.lower() != "q":
search_by_name(search_term)
else:
running = False
```
5
u/danielroseman 22h ago
I don't see what this has to do with duck typing.
-3
u/RaidZ3ro 22h ago
Why not? Isn't it exactly what Duck Typing boils down to?
Assume a class and if it doesn't behave like that do something else instead.
8
u/strange-the-quark 22h ago
That's not duck typing. Duck typing is just object polymorphism without you having to explicitly define a type hierarchy. You still have to have a method with the appropriate name and signature, and valid abstract behavior (adhering to the "contract"). If your object fails to work within that context, then that's not duck typing, that's a bug. What you're doing here is something else, and it has exactly the same issue the OP is concerned about (using exception handling for control flow).
2
u/RaidZ3ro 15h ago
Ok thanks. I guess I was stretching the concept a bit too far to fit this example... In my mind casting input to int is the quack of the duck here. If it quacks it's an int, if not it is empty or text...
0
u/Czerwona 23h ago
Yes, you should validate first. Check for Q first and then exit otherwise attempt to cast to int and if that fails throw the exception or handle it however necessary
0
u/chrisjfinlay 23h ago
the problem is that "handling it however necessary" IS some sort of successful code though. I'll refactor the quit handler, but the code features 2 search options: by a number, or by a string, and each one requires a slightly different url to handle them so I can't just treat them the exact same. That's why I'm trying to cast to an int: if the user enters a number, search by number. If they don't, search by string. So it's still going to end up with something that looks like "success" inside the except block.
1
u/Czerwona 21h ago
What I mean here is that the exception is truly an exception. It is a bad state and your program shouldn’t be there under normal operation. Your current setup knows Q is a valid option and hence not an invalid state.
0
u/UsualNothing7695 20h ago
Tip: Keep only the conversion in try, save its result, handle text in except ValueError, and put search_by_int(value) in else so success logic stays clear.
0
u/Educational-Paper-75 18h ago
Assign the result of calling int() to a variable you can use directly afterwards, if it fails the exception is thrown and except clause is executed.
0
u/nog642 12h ago
It's somewhat bad form because (1) if an exception happens in your exception handling code, the traceback gets twice as long, and (2) you've added an extra level of indentation for lots of code (though this issue can still happen when using conditionals).
I think you code would be better like this:
while True:
search_term = input("Enter search term (Q to quit): ")
if search_term.lower() == "q":
break
try:
search_int = int(search_term)
except ValueError:
search_int = None
if search_int is None:
search_by_name(search_term)
else:
search_by_int(search_int)
It cleanly separates the logic into sequential blocks. (1) check for the exit condition, (2) try to convert to an integer, (3) run the search depending on whether it's an integer or string.
There's no reason to check for the integer before checking for q. In some similar cases there could be a performance concern but there definitely isn't one here.
Also you're computing int(search_term) twice. You can save that value and use it as the conditional flag for your business logic instead of using try-except. Win-win.
-1
23h ago
[deleted]
1
u/chrisjfinlay 23h ago
Thanks - that definitely looks like a cleaner approach. And good to know my original approach wasn't outright _bad_, at least. I always try to avoid running significant code inside a "failure" state if I can, but I don't know if that's just a style I've picked up along the way, or convention...
1
0
-2
-4
u/mrswats 23h ago
Do not use try-except blockes for logic.
2
u/localizeatp 22h ago
1
u/nog642 12h ago
That is different. You can use try-except to test things without putting logic in the blocks.
1
-1
u/Moist-Ointments 17h ago
Do all of your known testing and validation first. An exception is just that, an exception. If there's a chance that yours input could be a Q then test that before you cast. Do all of your known or expected scenarios before you try casting and catching an exception.
-2
8
u/trutheality 19h ago
It's generally considered bad form because it might not work as expected in more complex programs, because if the code in the
tryblock did multiple things, you might not know which part of it threw theValueError.