1
\$\begingroup\$

I have a rather clunky function that returns a number based on certain values:

local = {'abs154': '4'}

w12,sv2,sv4,sv6,sv8,sv10,sv12=75,95,110,104,101,110,116 
supers = [["5", w12], ["6", w12], ["7", w12], ["8", w12], ["16", w12], ["17", w12], ["18", w12], ["9", sv2], ["11", sv2], ["12", sv2], ["13", sv2], ["14", sv2], ["15", sv2], ["19", sv4], ["23", sv4], ["24", sv4], ["25", sv4], ["26", sv4], ["28", sv6], ["29", sv6], ["30", sv6], ["31", sv6], ["32", sv6], ["33", sv6], ["35", sv8], ["36", sv8], ["37", sv8], ["38", sv8], ["39", sv8], ["40", sv8], ["41", sv8], ["42", sv8], ["43", sv8], ["44", sv8], ["45", sv8], ["46", sv8], ["47", sv8], ["48", sv8], ["49", sv8], ["50", sv8], ["52", sv10], ["53", sv10], ["55", sv10], ["57", sv10], ["58", sv10], ["59", sv10], ["60", sv10], ["61", sv10], ["62", sv10], ["63", sv10], ["64", sv10], ["65", sv10], ["66", sv10], ["68", sv2], ["71", sv12], ["72", sv12], ["73", sv12], ["74", sv12], ["75", sv12], ["76", sv12], ["77", sv12], ["78", sv12], ["79", sv12], ["80", sv12], ["81", sv12], ["82", sv12], ["83", sv12], ["84", sv12]] 

def Server(group): 
      try: sn = local[group] 
      except KeyError: 
            group = group.replace("-", "q") 
            fnv = float(int(group[0:min(5, len(group))], 36)) 
            lnv = group[6: (6 + min(3, len(group) - 5))] 
            if(lnv): 
                lnv = float(int(lnv, 36)) 
                lnv = max(lnv,1000) 
            else: 
                lnv = 1000 
            num = (fnv % lnv) / lnv 
            maxnum = sum(map(lambda x: x[1], supers)) 
            cumfreq = 0 
            sn = 0 
            for wgt in supers: 
                cumfreq += float(wgt[1]) / maxnum 
                if(num <= cumfreq): 
                    sn = int(wgt[0]) 
                    break 
      return "s" + str(sn) + ".website.com"

This all looks rather unnecessary. I would like to make it shorter and cleaner if possible.

\$\endgroup\$
5
  • \$\begingroup\$ sorry about the lack of code blocks. seems a bit different than stack \$\endgroup\$ Sep 3, 2013 at 0:52
  • \$\begingroup\$ Just click the code brackets in the editor toolbar while having the code selected \$\endgroup\$
    – Appleshell
    Sep 3, 2013 at 1:00
  • \$\begingroup\$ edited. was referred here by stack, yet no luck thus far \$\endgroup\$ Sep 3, 2013 at 1:02
  • 1
    \$\begingroup\$ @user2602977 Have you considered breaking this down into subfunctions with suggestive names? \$\endgroup\$
    – jaynp
    Sep 3, 2013 at 1:20
  • 1
    \$\begingroup\$ What is the motivation for this code? I suspect that it's excessively complicated because of an XY problem. \$\endgroup\$ Sep 3, 2013 at 5:15

2 Answers 2

2
\$\begingroup\$

some style issues to consider:

1) it's conventional to use ALL_CAPS for constants such as 'supers' and w1, etc.

2) for long term maintenance, it's also a good idea to use longer more descriptive names - not only is this hard to parse for the uninitiated, it's also prone to hard-to-catch typos which will be hard to spot. It's especially bad when you do multiple assignments on a single line.

3) supers could be a dictionary

4) you can simplify the range logic by excepting for incomplete groups:

<snip>
enter code here
group = group.replace("-", "q") 
fnv, lnv = None
try:    
   fnv = group[:5] 
   lnv = group[6:9]
except IndexError:
   raise ValueError "group string %s is malformed"
fn_value = int(fnv, 36)
ln_value = max(int(lnv, 36), 1000) or 1000

5) maxNum is always the same, it should be a constant or a cached module-level field:

informative_name = sum(map(lambda x: x[1], supers)) 

at the top will execute at module load and wont need to be recalced for every call.

\$\endgroup\$
0
\$\begingroup\$
  • Extract two functions from the except clause: one that extracts num from group, and one that looks up sn from super
  • Precomputing cumfreq into super would allow easy lookup with help from the bisect module.
  • group[0:min(5, len(group))] is same as group[:5] and group[6: (6 + min(3, len(group) - 5))] same as group[6:9]
  • There are redundant type conversions: lnv is made float but possibly replaced by the integer 1000, and sn goes from str to int to str.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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