3
\$\begingroup\$

I have roughly 40 or 50 functions that are similar to the one provided by the code below.

I kept all the common calculations between multiple functions separate such as service_coverage_area . I then kept all the specific calculations that are not repeated or needed by any other functions in the function that needs those calculations such as calculating f_sc.

My question is: Am I doing this right? as in is this the best practice to code utility functions? should I be separating all the mini calculations into separate functions but then have to deal with 200 functions instead of the current 50?

Note that the self is there cause this code is part of a class. I just copy and pasted 2 functions into this paste

import bisect

def service_coverage_area(self, mode):
  #simplified for codereview
  return 1


def some_calc(self, mode, street_type, grade, elderly_proportion, f_px):
    """ where:
          r = r_0 * f_sc * f_s * f_pop * f_px

          r_0 = ideal transit stop service radius
            400m for bus and 800m for rail / express bus and commuter
            needed variable: mode

          f_sc = street connectivity factor
            {'grid':1, 'hybrid':0.85, 'cul_de_sac':0.45}
            needed variable: street connectivity tye

          f_s = grade fator
            0.00 to 0.05 -> 1.00
            0.06 to 0.08 -> 0.95
            0.09 to 0.11 -> 0.80
            0.12 to 0.15 -> 0.65
            needed variable: grade

          f_pop population factor
            1 for normal population
            0.85 of 20% is elderly population
            needed variable:elderly_proportion

          f_px pedestrian crossing factor
            provided by another calculation
    """
    #calculate r_0
    r_0 = self.service_coverage_area(mode)

    #calculate f_sc
    if street_type == 'grid':
      f_sc = 1
    elif street_type == 'hybrid':
      f_sc = 0.85
    elif street_type == 'cul_de_sac':
      f_sc = 0.45

    #calculate f_s
    grades = [0.05, 0.08, 0.11]
    f_s_values = [1.00, 0.85, 0.80, 0.65]
    f_s_score = bisect.bisect(grades, grade)
    f_s = f_s_values[f_s_score]

    #calculate f_pop
    if elderly_proportion <= 0.2:
      f_pop = 1
    else:
      f_pop = 0.85

    return r_0 * f_sc * f_s * f_pop * f_px
\$\endgroup\$
5
\$\begingroup\$

Am I doing this right?

Almost.

as in is this the best practice to code utility functions?

These aren't "utility" functions. They appear to be central to your app. There are rarely "utility" functions in any app.

What we used to call "utility" functions are almost always part of a standard library.

should I be separating all the mini calculations into separate functions

Yes.

but then have to deal with 200 functions instead of the current 50?

You still have the calculations. You're just giving them names and keeping them separate. Giving them separate names makes them more reusable, easier to test and easier to find.

The most important thing here is to avoid creating a (nearly) useless class who's only job is to contain a bunch of functions.

Your code uses no instance variables and simply uses another function that happens to be in the class. Both of these could be @staticmethod method functions.

There's no reason -- in Python -- to create a class unless you have instance variables and a change in state of some kind.

[In Java, you must often create "all-static" classes because there's no other place to put stateless math like this.]

You can simply put these functions into a simple module and avoid the class definitions.

\$\endgroup\$
  • \$\begingroup\$ Seems to me quite a few python programmers forget that not everything needs to be a full fledged class. \$\endgroup\$ – Daniel Goldberg Jul 29 '11 at 10:52
  • \$\begingroup\$ @Daniel Goldberg: Bonus. I had someone on Stack Overflow who had a super-complex bit of staticmethod coding. When I suggested it was just a function, they insisted that using a bare function violated some OO principle or other. \$\endgroup\$ – S.Lott Jul 29 '11 at 10:56
  • \$\begingroup\$ Thanks a lot for the advice, I spent the better half of the evening splitting the file into multiple modules. Still working on the tests and all; however, time will tell on how my ability to maintain these modules. Thanks for the reply though. \$\endgroup\$ – dassouki Jul 29 '11 at 12:21
  • \$\begingroup\$ I agree about your comment on classes; therefore, I won't be using classes or @staticmethods \$\endgroup\$ – dassouki Jul 29 '11 at 12:21
  • 1
    \$\begingroup\$ "multiple modules"? Why waste time on that? They're all related, right? If you thought they belonged in one class, then they definitely belong in one module. \$\endgroup\$ – S.Lott Jul 29 '11 at 12:23
1
\$\begingroup\$
#calculate r_0
r_0 = self.service_coverage_area(mode)

I recommend a more descriptive variable name then r_0 (some for all your variables)

#calculate f_sc
if street_type == 'grid':
  f_sc = 1
elif street_type == 'hybrid':
  f_sc = 0.85
elif street_type == 'cul_de_sac':
  f_sc = 0.45

Use a dictionary f_sc = MYDICT[street_type]

#calculate f_s
grades = [0.05, 0.08, 0.11]
f_s_values = [1.00, 0.85, 0.80, 0.65]

constants are better placed at the global/class level rather than inside the function.

\$\endgroup\$
  • \$\begingroup\$ Concerning the first point: I defined r_0 in the doc string, with respect to the code r_0 is usually known. I agree and thanks about the dict. With respect to teh constants, those will eventually move into the db. I have a #TODO on top of each one to remind me to move them. \$\endgroup\$ – dassouki Jul 29 '11 at 13:41
  • \$\begingroup\$ @dassouki, yes you define it. I'd still give it a better name. \$\endgroup\$ – Winston Ewert Jul 29 '11 at 21:20

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.