Simple Function in python [closed]

Question

I was wondering if this code can be coded better in terms of semantics or design. Should I return the LOS as numbers? should I have the constants be read from the db in case the user or I want to change them in the future?

def get_LOS(headway = None, frequency = None, veh_per_hr = None):
  if frequency != None and headway = veh_per_hr == None:
    headway = 1 / frequency

  if headway!= None or veh_per_hr != None:
    if headway 10 < or veh_per_hr > 6:
      return 'A'
    elif 10 <= headway < 15 or 5 < veh_per_hr <= 6:
      return 'B'
    elif 15 <= headway < 20 or 3 < veh_per_hr <= 5:
      return 'C'
    elif 20 <= headway < 31 or 2 < veh_per_hr <= 3:
      return 'D'
    elif 31 <= headway < 60 or 1 <= veh_per_hr < 2:
      return 'E'
    elif headway >= 60 or veh_per_hr < 1:
      return 'F'
    else:
      return 'Error'

Answer 1

def get_LOS(headway = None, frequency = None, veh_per_hr = None):

You have no docstring. That would helpful in explaining what the parameters are doing.

  if frequency != None and headway = veh_per_hr == None:

When checking wheter something is none it is best to use is None or is not None

    headway = 1 / frequency

The problem is that if someone passes frequency along with one of the other parameters, this function will go merrily onwards and probably not even produce an error. I recommend having a get_LOS_from_frequency function which takes the frequency and then calls this function.

  if headway!= None or veh_per_hr != None:
    if headway 10 < or veh_per_hr > 6:

I'm pretty sure that won't compile

       return 'A'
    elif 10 <= headway < 15 or 5 < veh_per_hr <= 6:
      return 'B'
    elif 15 <= headway < 20 or 3 < veh_per_hr <= 5:
      return 'C'
    elif 20 <= headway < 31 or 2 < veh_per_hr <= 3:
      return 'D'
    elif 31 <= headway < 60 or 1 <= veh_per_hr < 2:
      return 'E'
    elif headway >= 60 or veh_per_hr < 1:
      return 'F'

I'd store these values in a list which makes it easy to pull them from configuration at a later date if neccesary.

    else:
      return 'Error'

Don't report error by returning strings. Throw an exception, or at least assert False.

How I'd do this:

import bisect
def get_los_from_frequency(frequency):
    return get_los(1 / frequency)

HEADWAY_LIMITS = [10, 15, 20, 31, 60]
VEH_PER_HR_LIMITS = [1,2,3,5,6]
GRADES = "ABCDEF"

def get_los(headway = None, veh_per_hr = None):
    if headway is None:
        headway_score = len(GRADES)
    else:
        headway_score = bisect.bisect_left(HEADWAY_LIMITS, headway)

    if veh_pr_hr is None:
        veh_pr_hr_score = len(GRADES)
    else:
        veh_pr_hr_score = len(GRADES) - bisect.bisect_left(VEH_PR_HR_LIMITS, veh_pr_hr)

    return GRADES[min(headway_score, veh_pr_hr_score)]

Answer 2

Rather than:

if frequency != None and headway = veh_per_hr == None:
    headway = 1 / frequency

You should move the assignment out of the if statement:

if frequency is not None:
    headway = veh_per_hr
    if veh_per_hr is None:
        headway = 1 / frequency

This will make it more clear what you're doing without making a future reader do a double-take when they see the assignment embedded in the middle of an if statement. It also might point out that this might be better written as:

headway = veh_per_hr
if veh_per_hr is None and frequency is not None:
    headway = 1 / frequency

As an aside, if you want to know why to use is None or is not None instead of == None or != None, one reason is:

>>> import timeit
>>> timeit.Timer('x=1; x != None').timeit()
0: 0.18491316633818045
>>> timeit.Timer('x=1; x is not None').timeit()
1: 0.17909091797979926

Another could be the fact that it's much easier to accidentally type = instead of == and have an unnoticed assignment slip by in your conditional (which is another argument for never doing that on purpose as a later maintainer might mistake that for a typo).