3
\$\begingroup\$

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'
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Edit so that the code is readable and correct. Also you might want to explain a little more about what get_LOS is actually trying to accomplish and what the values 'A' - 'F' mean. \$\endgroup\$ Jul 26 '11 at 21:20
  • 2
    \$\begingroup\$ This code can't run in this form. headway = veh_per_hr == None is illegal as is headway 10 <. Please be sure that code you want reviewed has a small chance of actually running. And if it doesn't run, please use StackOverflow to find and fix the problems. \$\endgroup\$
    – S.Lott
    Jul 27 '11 at 21:20
  • \$\begingroup\$ The current question title is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Jun 4 '18 at 9:29
8
\$\begingroup\$
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)]
\$\endgroup\$
1
  • \$\begingroup\$ This was very helpful especially with the bisect and lists :) \$\endgroup\$
    – dassouki
    Jul 27 '11 at 11:31
2
\$\begingroup\$

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).

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.