current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I have a bunch of functions that return strings based upon score ranges. Then I have another function that sums everything up and assigns a numeric weight.

@property
def risk_status(self):
    """ This function assigns numeric weight to risks. It returns an over all integer score. """

    # Build a string of risks, ie "High Risk@At Risk@@Extreme Risk"
    raw_string_of_risks = self.si_risk + '@' + self.aggbs_risk + '@' + self.cfimb_risk + '@' + self.cfiab_risk + '@' + self.cei_risk

    # Build a list ["High Risk", "At Risk", "Extreme Risk"]
    risks = raw_string_of_risks.split('@')

    # Formula for over all risk status. ["High Risk", "At Risk", "Extreme Risk"] => 2+1+3 = 6
    status = risks.count("At Risk")*1 + risks.count("High Risk")*2 + risks.count("Extreme Risk")*3

    return status

I'm afraid with a gazillion records this property might slow things down. It will never get a string longer than 65 characters to parse, but will doing this over and over again really slow things down?

share|improve this question
1  
I don't know if it will speed things up a lot, but instead of building your raw_string_of_risks and then breaking it into a list, why don't you just create the list ... risks = [self.si_risk, self.aggbs_risk, ...] –  jcfollower Apr 4 '14 at 20:48
    
That is much better! Slapping my forehead! –  broinjc Apr 7 '14 at 18:26

1 Answer 1

up vote 3 down vote accepted

Concatenating an splitting seems like you're making extra work for yourself. I suggest summing the results of a string-to-integer translation.

from collections import defaultdict

_RISK_SCORE = defaultdict(int, [('Extreme Risk', 3), ('High Risk', 2), ('At Risk', 1)])

@property
def risk_status(self):
    """ This function assigns numeric weight to risks. It returns an overall integer score. """
    return sum(self._RISK_SCORE[r] for r in (
        self.si_risk,
        self.aggbs_risk,
        self.cfimb_risk,
        self.cfiab_risk,
        self.cei_risk,
    ))
share|improve this answer
    
Why defaultdict? It will mask any typos. –  Suor Apr 10 '14 at 7:49
    
@Suor The author never explicitly stated that self.si_risk etc. would only take on those three string values. This code, with defaultdict, most closely mimics the behaviour of the original code, which also masks typos. –  200_success♦ Apr 10 '14 at 7:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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