4
\$\begingroup\$

I created a Python class called Score. I intend to use it to assign scores depending on two parameters.

  1. the type of score, score_name, and
  2. the user_input.

If I want to get the score of a loan term I will enter 'loan_term' as the score_name and 36 as user_input. The method Score.get_score then calculates the score and returns a value.

The idea is to be able to re-use this class as and when needed and to be able to modify the code outside of the main application code.

I would like a review of the implementation of my class. Have I deviated from standards or is there a better way to implement this?

class Score:
    def __init__(self, score_name):
        self.score_name = score_name

    def get_score_name(self):
        return self.score_name

    def get_score(get_score_name, user_input):
        # loan term score grading
        if get_score_name == 'loan_term' and user_input <= 18:
            i = 5
        elif get_score_name == 'loan_term' and user_input <= 24:
            i = 4
        elif get_score_name == 'loan_term' and user_input <= 48:
            i = 3
        # loan reason score grading
        elif get_score_name == 'loan_reason' and user_input == 0:
            i = 3.5
        elif get_score_name == 'loan_reason' and user_input == 1:
            i = 2.0
        elif get_score_name == 'loan_reason' and user_input == 2:
            i = 1.5
        elif get_score_name == 'loan_reason' and user_input == 3:
            i = 2.5
        elif get_score_name == 'loan_reason' and user_input == 4:
            i = 1.5
        elif get_score_name == 'loan_reason' and user_input == 5:
            i = 2.0
        elif get_score_name == 'loan_reason' and user_input == 6:
            i = 0.5
        # job security score grading
        elif get_score_name == 'job_security' and user_input == 0:
            i = 1.0
        elif get_score_name == 'job_security' and user_input == 1:
            i = 2.0
        elif get_score_name == 'job_security' and user_input == 2:
            i = 3.0
        elif get_score_name == 'job_security' and user_input == 3:
            i = 4.0
        elif get_score_name == 'job_security' and user_input == 4:
            i = 5.0
        # average apr score grading
        elif get_score_name == 'average_apr' and user_input <= -1.0:
            i = 5
        elif get_score_name == 'average_apr' and user_input <= -0.6:
            i = 4
        elif get_score_name == 'average_apr' and user_input < 0.5:
            i = 3
        elif get_score_name == 'average_apr' and user_input <= 1.0:
            i = 2
        elif get_score_name == 'average_apr' and user_input > 1.0:
            i = 1
        # monthly expenses score grading
        elif get_score_name == 'monthly_expenses' and user_input < 0.10:
            i = 5
        elif get_score_name == 'monthly_expenses' and user_input < 0.20:
            i = 4
        elif get_score_name == 'monthly_expenses' and user_input < 0.30:
            i = 3
        elif get_score_name == 'monthly_expenses' and user_input < 0.40:
            i = 1
        elif get_score_name == 'monthly_expenses' and user_input > 0.40:
            i = 0
        # monthly surplus score grading
        elif get_score_name == 'monthly_surplus' and user_input < 0.20:
            i = 5
        elif get_score_name == 'monthly_surplus' and user_input < 0.35:
            i = 4
        elif get_score_name == 'monthly_surplus' and user_input < 0.45:
            i = 3
        elif get_score_name == 'monthly_surplus' and user_input <= 0.60:
            i = 2
        elif get_score_name == 'monthly_surplus' and user_input > 0.60:
            i = 1
        else:
            i = 0

        return i
# loan term test

loan_term_score = Score.get_score(Score('loan_term').get_score_name(), 21)
print(f'loan term  score: {loan_term_score}')


# loan reason test

loan_reason_score = Score.get_score(Score('loan_reason').get_score_name(), 2)
print(f'loan reason score: {loan_reason_score}')

# job security test

job_security_score = Score.get_score(Score('job_security').get_score_name(), 3)
print(f'job security score: {job_security_score}')


# average apr test

average_apr_score = Score.get_score(Score('average_apr').get_score_name(), 1.0)
print(f'average apr score: {average_apr_score}')

# monthly expenses test

monthly_expenses_score = Score.get_score(Score('monthly_expenses').get_score_name(), 0.15)
print(f'monthly expenses score: {monthly_expenses_score}')

# monthly surplus test

monthly_surplus_score = Score.get_score(Score('monthly_surplus').get_score_name(), 0.55)
print(f'monthly surplus score: {monthly_surplus_score}')
\$\endgroup\$
1
  • \$\begingroup\$ In the average_apr section, did you really mean < 0.5 or did you mean <= 0.5 the way all the rest of the cases are? \$\endgroup\$
    – aghast
    Commented Aug 13, 2020 at 14:53

2 Answers 2

1
\$\begingroup\$

The Class

  1. This is not how you use methods on a class. Firstly get_score is defined as if it is a static method but it has not been wrapped in a @staticmethod meaning that it is an instance method. Whilst you've got it so that it works because you're using it via the type not an instance. This is nothing but Python abuse.

    @staticmethod
    def get_score(get_score_name, user_input):
        ...
    
  2. Why is this a staticmethod at all?

    Score.get_score(Score('loan_term').get_score_name(), 21)
    

    If you make it an instance method then you can remove the need for the get_score_name method. As you can replace all get_score_name with self.score_name except the argument which will be self.

    def get_score(self, user_input):
        if self.score_name == 'loan_term' and user_input <= 18:
    

    This gets rid of the get_score_name.

    Score.get_score(Score('loan_term'), 21)
    

    However this is still Python abuse. And so we can simplify the usage further.

    Score('loan_term').get_score(21)
    
  3. Why is this even a class? Why not just delete all the code except get_score and you can just use it without lots of noise that adds nothing to your code.

    get_score('loan_term', 21)
    

Design

The idea is to be able to re-use this class as and when needed and to be able to modify the code outside of the main application code.

We can see that you have 6 groups; loan_term, loan_reason, job_security, average_apr, monthly_expenses and monthly_surplus.

From this we can see your design is bad. Your function breaks the Single Responibility Principle, SRP. We can see this is making life harder right now as you have lots of if get_score_name == '...' and checks that can just disappear if you make these stand alone functions. Additionally this will lead you further down a path that will make your code even worse to maintain. You should just make these stand alone functions.

def loan_term(user_input):
    if user_input <= 18:
        return 5
    elif user_input <= 24:
        return 4
    elif user_input <= 48:
        return 3
    else:
        return 0


def loan_reason(user_input):
    if user_input == 0:
        return 3.5
    elif user_input == 1:
        return 2.0
    elif user_input == 2:
        return 1.5
    elif user_input == 3:
        return 2.5
    elif user_input == 4:
        return 1.5
    elif user_input == 5:
        return 2.0
    elif user_input == 6:
        return 0.5
    else:
        return 0.0


def job_security(user_input):
    if user_input == 0:
        return 1.0
    elif user_input == 1:
        return 2.0
    elif user_input == 2:
        return 3.0
    elif user_input == 3:
        return 4.0
    elif user_input == 4:
        return 5.0
    else:
        return 0.0


def average_apr(user_input):
    if user_input <= -1.0:
        return 5
    elif user_input <= -0.6:
        return 4
    elif user_input < 0.5:
        return 3
    elif user_input <= 1.0:
        return 2
    elif user_input > 1.0:
        return 1
    else:
        return 0


def monthly_expenses(user_input):
    if user_input < 0.10:
        return 5
    elif user_input < 0.20:
        return 4
    elif user_input < 0.30:
        return 3
    elif user_input < 0.40:
        return 1
    else:
        return 0


def monthly_surplus(user_input):
    if user_input < 0.20:
        return 5
    elif user_input < 0.35:
        return 4
    elif user_input < 0.45:
        return 3
    elif user_input <= 0.60:
        return 2
    else:
        return 1


# loan term test

loan_term_score = loan_term(21)
print(f'loan term  score: {loan_term_score}')


# loan reason test

loan_reason_score = loan_reason(2)
print(f'loan reason score: {loan_reason_score}')

# job security test

job_security_score = job_security(3)
print(f'job security score: {job_security_score}')


# average apr test

average_apr_score = average_apr(1.0)
print(f'average apr score: {average_apr_score}')

# monthly expenses test

monthly_expenses_score = monthly_expenses(0.15)
print(f'monthly expenses score: {monthly_expenses_score}')

# monthly surplus test

monthly_surplus_score = monthly_surplus(0.55)
print(f'monthly surplus score: {monthly_surplus_score}')
\$\endgroup\$
0
\$\begingroup\$

This answer is predominantly an extension of Peilonrayz's answer. When you look at what your functions actually do, they fall into one of two categories:

  • A direct 1:1 lookup
  • A range-based search

You can simplify these even further by doing something like this:

  1. Create a simple method to lookup a value in a range (class RangeLookup)
  2. Fix average_apr to realize that your >= 1.0 case is actually the same as your else case
  3. Replace 1:1 lookups with some_dictionary.get(user_input, default_value)
  4. Replace range-based searches with some_range_lookup.get_value_for_point(user_input)

I ended up with something like this:

class RangeLookup:
    def __init__(self, delimiters, default_value):
        self.default = default_value
        self.ranges = sorted(
            delimiters, key=lambda delim: delim[0]
        )

    def get_value_for_point(self, point):
        for (delimiter, value, inclusive) in self.ranges:
            if delimiter > point or (
                inclusive and delimiter >= point
            ):
                return value

        return self.default

_loan_term_lookup = RangeLookup(
    [(18, 5, True), (24, 4, True), (48, 3, True)], 0
)
def loan_term(user_input):
    return _loan_term_lookup.get_value_for_point(user_input)

_loan_reason_lookup = {
    0: 3.5,
    1: 2.0,
    2: 1.5,
    3: 2.5,
    4: 1.5,
    5: 2.0,
    6: 0.5,
}
def loan_reason(user_input):
    return _loan_reason_lookup.get(user_input, 0.0)

_job_security_lookup = {
    0: 1.0,
    1: 2.0,
    2: 3.0,
    3: 4.0,
    4: 5.0,
}
def job_security(user_input):
    return _job_security_lookup.get(user_input, 0.0)

_average_apr_lookup = RangeLookup(
    [
        (-1.0, 5, True),
        (-0.6, 4, True),
        (0.5, 3, False),
        (1.0, 2, True),
    ],
    1,
)
def average_apr(user_input):
    return _average_apr_lookup.get_value_for_point(
        user_input
    )

_monthly_expenses_lookup = RangeLookup(
    [
        (0.1, 5, False),
        (0.2, 4, False),
        (0.3, 3, False),
        (0.4, 1, False),
    ],
    0,
)
def monthly_expenses(user_input):
    return _monthly_expenses_lookup.get_value_for_point(
        user_input
    )

_monthly_surplus_lookup = RangeLookup(
    [
        (0.2, 5, False),
        (0.35, 4, False),
        (0.45, 3, False),
        (0.6, 2, True),
    ],
    1,
)
def monthly_surplus(user_input):
    return _monthly_surplus_lookup.get_value_for_point(
        user_input
    )
\$\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.