-1
\$\begingroup\$

In the case where a list might or might not exist, and if it exists a value should be returned, else do not return anything, is there a better way than the below.

def dollar_band_splits(x, lst_num=None, lst_dollars=None, lst_approved=None):
    if x >= 0 and x <=125000:
        return lst_num[0], lst_dollars[0], lst_approved[0] if lst_approved else ''
    if x >125000  and x <=250000:
        return lst_num[1], lst_dollars[1], lst_approved[1] if lst_approved else ''
    if x >250000  and x <=500000:
        return lst_num[2], lst_dollars[2], lst_approved[2] if lst_approved else ''
    if x >= 500000 and x <=1000000:
        return lst_num[3], lst_dollars[3], lst_approved[3] if lst_approved else ''
    if x >= 1000000 and x <=5000000:
        return lst_num[4], lst_dollars[4], lst_approved[4] if lst_approved else ''
    else:
        return lst_num[5], lst_dollars[5], lst_approved[5] if lst_approved else ''
\$\endgroup\$
  • \$\begingroup\$ This is kind of broken, unless you're assuming that if lst_approved is provided all of them are provided. You're also returning different types depending on the input, which is icky. \$\endgroup\$ – Dannnno Nov 2 '17 at 21:00
  • \$\begingroup\$ You also return the 6th item if x is non-positive, which seems like a bug. \$\endgroup\$ – Dannnno Nov 2 '17 at 21:04
  • \$\begingroup\$ @Dannnno Thanks. The other 3 arguments will always be provided, but I would like a general strategy on how to deal with the case where a list might exist. The reason why I am returning the '' is my understanding is that for the ternary you need an else statement. The 6th item is in the case of values >5000000. I guess I should be explicit about that. I am making assumptions about the data (I know the values are all positive numbers). \$\endgroup\$ – ivan7707 Nov 2 '17 at 21:06
  • \$\begingroup\$ I would look into numpy searchsorted as an alternative to if statements. You can also use a dictionary to find the key within desired bounds.. \$\endgroup\$ – MPath Nov 3 '17 at 2:50
5
\$\begingroup\$

Your code is confusing; at a first glance I misread the returned tuple as being dependent on the tuple; only after looking again did I realize that it was just the final parameter being affected. This is super confusing, and you should avoid doing it this way. Consider the following:

def dollar_band_splits(x, num, dollars, approved=None):
    if x < 0:
        raise ValueException("X is less than 0")

    thresholds = [125000, 250000, 500000, 1000000, 5000000]
    approved = approved or [''] * len(thresholds)+1

    if len(num) < len(thresholds) + 1:
        raise ValueException("The num list doesn't have enough elements")
    if len(dollars) < len(thresholds) + 1:
        raise ValueException("The dollars list doesn't have enough elements")
    if len(approved) < len(thresholds) + 1:
        raise ValueException("The approval list doesn't have enough elements")

    zipped = zip(num, dollars, approved)
    for i, threshold in enumerate(thresholds):
        if x < threshold:
            return zipped[i]
    return zipped[len(thresholds)]

I've done a few things worth pointing out.

  • I stopped prepending lst to the name of your parameters, because you don't need to say their type. Just give them better names overall.
  • If num and dollars are always provided then don't allow them to be None.
  • You say you won't ever have a negative value of x, but defensive programming says you should throw an exception if it is.
  • If the approved list isn't provided, then I use a list of empty values so the zipping works correctly.
  • I explicitly throw some exceptions if you get poorly formed data in your lists, i.e. if you don't have enough data.

Beyond that, we just iterate over the thresholds until we find one we don't exceed, and return that one. Otherwise, we default to the (presumably) last element.

It's possible this will take a bit of a performance hit, especially if you add a bunch of thresholds, but the benefit in readability is almost certainly worth it unless it makes it too slow. It isn't too slow until it exceeds some measurable threshold that is set by your business needs.


Everything here and below is part of my original answer, which was incorrect as per the misunderstanding I mentioned above. Left for posterity

Your code is super repetitive, and could be easily cleaned up like so:

def dollar_band_splits(x, num=None, dollars=None, approved=None):
    if approved == None or x < 0:
        return None
    thresholds = [125000, 250000, 500000, 1000000, 5000000]
    zipped = zip(num, dollars, approved)
    for i, threshold in enumerate(thresholds):
        if x < threshold:
            return zipped[i]
    return zipped[len(thresholds)]

I fixed a few things here:

  • I stopped appending lst to the name of your parameters, because you don't need to say their type. Just give them better names overall.
  • I returned None instead of '' when there wasn't a value, because that makes more sense to me
  • I fixed what was probably a bug by returning None when x < 0. In the comments you indicated that you won't have any negative values - in that case I would throw an exception if x < 0.
  • I check right away if the lists aren't present, in which case you can break out early.

Otherwise this just looks at all of the thresholds and if they don't meet them, return the appropriate value. It's possible this will take a bit of a performance hit, especially if you add a bunch of thresholds, but the benefit in readability is almost certainly worth it unless it makes it too slow.

Too slow should be a measurable threshold that is determined by what is acceptable for your application, not a gut feeling.

You probably also want to throw an exception if the length of the arrays isn't at least len(thresholds). It'll do this already, but I prefer exceptions you throw yourself instead of the default ones, as it allows you to put specific information in about the cause and how to fix it.

\$\endgroup\$
  • \$\begingroup\$ Thanks! I need to always return num and dollars, so I included: if approved: zipped = list(zip(num, dollars, approved)) else: zipped = list(zip(num, dollars)) \$\endgroup\$ – ivan7707 Nov 2 '17 at 21:31
  • \$\begingroup\$ @ivan7707 I rewrote my answer a bit as my initial understanding was incorrect. \$\endgroup\$ – Dannnno Nov 3 '17 at 13:26
5
\$\begingroup\$

Your question doesn't explain the motivation and intended usage of this function, so I'll have to make some assumptions and guesses.

First, note that lst_num and lst_dollars should be mandatory parameters. If you call dollar_band_splits() with these parameters defaulting to None, then the function is going to crash.

Next, note that you are sloppy with your inequalities: if x >= 500000 and if x >= 1000000 would be be better written as if x > 500000 and if x > 1000000, respectively.

Third, it's rather bizarre that the case of negative x values produces the same result as the case of x exceeding 5 million. I'm going to assume that that is a bug, but I can't recommend a fix without knowing what the desired error-handling behaviour is.

Python supports double-ended inequalities, so you could write if 125000 < x <= 250000. That would be an improvement, but it would still leave a lot of repetition in the code.

Suggested solution

def dollar_band_splits(x, lst_num, lst_dollars, lst_approved=[''] * 6):
    THRESHOLDS = [125000, 250000, 500000, 1000000, 5000000]
    i = sum(x > threshold for threshold in THRESHOLDS)
    return lst_num[i], lst_dollars[i], lst_approved[i]

This simple function is almost equivalent to your code, but differs in two ways:

  • Negative values of x are treated the same as small positive values of x. If that is unacceptable, then insert an if x < 0: special case as the first statement.
  • If called with a falsy parameter value for lst_approved, such as [] (an empty list), 0, False, or None, then the default won't kick in, and the function would crash when referencing lst_approved[i]. That shouldn't be a problem, as long as callers aren't abusive.
\$\endgroup\$
  • \$\begingroup\$ Thank you for the helpful response. The reason why I am not treating negatives specifically is that the data is coming from a SQL script that ensures positive values. I always thought that one should never use mutable default arguments in python. \$\endgroup\$ – ivan7707 Nov 3 '17 at 0:57
  • \$\begingroup\$ This function never mutates the default parameter, and list's string elements are immutable. \$\endgroup\$ – 200_success Nov 3 '17 at 0:59

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.