4

I've just written a chunk of code that strikes me as being far more nested than is optimal. I'd like advice on how to improve the style of this, particularly so that it conforms more with "Flat is better than nested."

for app in apps: if app.split('.', 1)[0] == 'zc': #only look for cron in zc apps try: a = app + '.cron' __import__(a) m = sys.modules[a] try: min = m.cron_minute() for job in min: k.add_interval_task(job[0], 'minute task', r(M_LB, M_UB), 60*job[1], kronos.method.threaded, (), ()) except AttributeError: #no minute tasks pass try: hour = m.cron_hour() for job in hour: k.add_daytime_task(job[0], 'day task', range(1, 8), None, (job[1], r(H_LB, H_UB)), kronos.method.threaded, (), ()) except AttributeError: #no hour tasks pass except ImportError: #no cron jobs for this module pass

Edit: Combining the suggestions from below, here's my rewritten form.

for app in apps: if app.split('.', 1)[0] != 'zc': #only look for cron in zc apps continue try: a = app + '.cron' __import__(a) except ImportError: #no cron jobs for this module, continue to next one continue m = sys.modules[a] if hasattr(m, 'cron_minute'): min = m.cron_minute() for job in min: k.add_interval_task(job[0], 'minute task', r(M_LB, M_UB), 60*job[1], kronos.method.threaded, (), ()) if hasattr(m, 'cron_hour'): hour = m.cron_hour() for job in hour: k.add_daytime_task(job[0], 'day task', range(1, 8), None, (job[1], r(H_LB, H_UB)), kronos.method.threaded, (), ())
Improve this question

4 Answers 4

Reset to default
8

The main problem is that your try clauses are too broad, particularly the outermost one: with that kind of habit, you WILL sooner or later run into a mysterious bug because one of your try/except has accidentally hidden an unexpected exception "bubbling up" from some other function you're calling.

So I'd suggest, instead:

for app in apps: if app.split('.', 1)[0] != 'zc': #only look for cron in zc apps continue try: a = app + '.cron' __import__(a) except ImportError: #no cron jobs for this module continue # etc etc

As an aside, I'm also applying "flat is better than nested" in another way (not dependent on any try/except) which is "if I have nothing more to do on this leg of the loop, continue [i.e. move on to the next leg of the loop] instead of "if I have something to do:" followed by a substantial amount of nested code. I've always preferred this style (of if/continue or if/return) to nested if's in languages that supply functionality such as continue (essentially all modern ones, since C has it;-).

But this is a simple "flat vs nested" style preference, and the meat of the issue is: keep your try clauses small! Worst case, when you just can't simply continue or return in the except clause, you can use try/except/else: put in the try clause only what absolutely MUST be there -- the tiny piece of code that's likely and expected to raise -- and put the rest of the following code (the part that's NOT supposed nor expected to raise) in the else clause. This doesn't change the nesting, but DOES make a huge difference in lowering the risk of accidentally hiding exceptions that are NOT expected!

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Thank you, this is exactly what I needed. I had forgotten about using continue in this manner. Your advice about keeping the try as small as possible makes sense. Using continue allows me to do that. It's much more explicit in the rewritten form.
1

I wonder, if the jobs for each time unit is actually broken, would they raise an AttibuteError, or some other exception? In particular, if there's something about a job that is really busted, you probably aught not to catch them.

Another option that can help is to wrap only the offending code with a try-catch, putting the exception handler as close to the exception as possible. Here's a stab: 

for app in apps: if app.split('.', 1)[0] == 'zc': #only look for cron in zc apps try: a = app + '.cron' __import__(a) m = sys.modules[a] except ImportError: #no cron jobs for this module #exception is silently ignored #since no jobs is not an error continue if hasattr(m, "cron_minute"): min = m.cron_minute() for job in min: k.add_interval_task(job[0], 'minute task', r(M_LB, M_UB), 60*job[1], kronos.method.threaded, (), ()) if hasattr(m, "cron_hour"): hour = m.cron_hour() for job in hour: k.add_daytime_task(job[0], 'day task', range(1, 8), None, (job[1], r(H_LB, H_UB)), kronos.method.threaded, (), ())

notice there is only one exception handler here, which we handle by correctly ignoring. since we can predict the possibility of there not being one attribute or another, we check for it explicitly, which helps to make the code a bit clearer. Otherwise, it's not really obvious why you are catching the AttributeError, or what is even raising it.

Improve this answer

1 Comment

Ah thank you! Using hasattr() here is a much cleaner approach than try/except.
0

You can create a function that performs the main logic, and another function which invokes that function, wrapping it with the try...except statements. Then, in the main application, you can just invoke those function which already handle the exceptions. (Based on the recommendations of the "Clean Code" book).

Improve this answer

1 Comment

This code only gets called once. I could write a couple functions to do it, but that's really only generating spaghetti code, rather than actually improving the layout.
0

Well, the trick here is to figure out if they are broken. That is the handling part of exception handling. I mean, at least print a warning that states the comment's assumption. Worrying about excessive nesting before the actual handling seems like you are getting ahead of yourself. Worry about being right before being stylish.

Improve this answer

1 Comment

You've misunderstood what the code does. It's supposed to do nothing if the exceptions are caught - it's not an error if the things that are being checked for don't exist (in fact, they won't, in most cases). I'd prefer not to have warnings printed for every instance of an optional function not being found.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.