2
\$\begingroup\$

I have a class Scholar which is basically a teacher in a university. I have to calculate the yearly salary of a scholar instance where the formula is 12-month salary + bonus
the bonus is determined by the academic_position
if the teacher is:

  • a Professor --> bonus = 70000
  • an Associate Professor --> bonus = 50000
  • an Assistant Professor --> bonus = 30000

I want to keep the bonus in a separate variable so i implemented it in __init__, but it looks messy. not sure if it is the way to do or not. do you have any suggestion on my code? Is there a way to improve readibility? what normally a pythonista do with this kind of problem?

class Scholar:
  def __init__(self, name, salary, academic_position):

    self.name = name
    self.salary = salary
    self.academic_position = academic_position

    if self.academic_position == 'Professor':
      self.bonus = 70000
    elif self.academic_position == 'Associate Professor':
      self.bonus = 50000
    elif self.academic_position == 'Assistance Professor':
      self.bonus = 30000
\$\endgroup\$
1
  • \$\begingroup\$ It's likely that it doesn't even make sense to set such an instance variable in the constructor. But we can't really advise you properly based on the tiny code excerpt you've shown here. \$\endgroup\$ Nov 20 at 17:28
1
\$\begingroup\$

If you have to stick to using this specific structure, I'd do it like this:

class Scholar:
    _ACADEMIC_POSITIONS_BONUS_MAP = {
        'Professor': 70000,
        'Associate Professor': 50000,
        'Assistance Professor': 30000,
    }

    def __init__(self, name, salary, academic_position):
        self.name = name
        self.salary = salary
        self.academic_position = academic_position

    @property
    def bonus(self):
        for academic_position, bonus in self._ACADEMIC_POSITIONS_BONUS_MAP.items():
            if self.academic_position == academic_position:
                return bonus

        raise ValueError(
            f"Invalid academic position. Allowed: "
            f"{', '.join(self._ACADEMIC_POSITIONS_BONUS_MAP)}"
        )

And as other have mentioned, instead of iterating through the dict itself you can just do this:

class Scholar:
    # ...

    @property
    def bonus(self):
        return self._ACADEMIC_POSITIONS_BONUS_MAP[self.academic_position]

We've created a map between each type of academic_position and its specific bonus and a @property where we return the bonus.

And then you can use it like this:

professor = Scholar("John Doe", 30000, "Professor")
associate_professor = Scholar("Associate John Doe", 30000, "Associate Professor")
assistance_professor = Scholar("Assistance John Doe", 30000, "Assistance Professor")


print(professor.bonus)
print(associate_professor.bonus)
print(assistance_professor.bonus)

Now, when you need to add another academic_position, you just have to modify _ACADEMIC_POSITIONS_BONUS_MAP.

Next, in order to calculate the yearly salary, just create another method within your class:

class Scholar:
    # ...

    def get_yearly_salary(self):
        # Note: multiply the salary with 12 if it's monthly-based
        return self.salary + self.bonus

Note: I'm not sure if @property is the best way of doing this. You could've just done something along the lines:

class Scholar:
    _ACADEMIC_POSITIONS_BONUS_MAP = {
        'Professor': 70000,
        'Associate Professor': 50000,
        'Assistance Professor': 30000,
    }

    def __init__(self, name, salary, academic_position):
        self.name = name
        self.salary = salary
        self.academic_position = academic_position

        self.bonus = self.get_bonus()

    def get_bonus(self):
        for academic_position, bonus in self._ACADEMIC_POSITIONS_BONUS_MAP.items():
            if self.academic_position == academic_position:
                return bonus

        raise ValueError(
            f"Invalid academic position. Allowed: "
            f"{', '.join(self._ACADEMIC_POSITIONS_BONUS_MAP)}"
        )
\$\endgroup\$
8
  • \$\begingroup\$ @ Grajdeano Alex thanks you so much for your answers. so basically @property is like an attribute which has immediate invoked call? we can access simply just like it is an instance variable? \$\endgroup\$
    – tmo
    Nov 19 at 13:53
  • \$\begingroup\$ and you're suggesting that I should write get_bonus() instead of the @property bonus in order to protect the variable from changing outside the class? \$\endgroup\$
    – tmo
    Nov 19 at 13:56
  • \$\begingroup\$ Basically yes. For a deeper read have at this \$\endgroup\$ Nov 19 at 14:10
  • 2
    \$\begingroup\$ Iterating over a dict to check a value for equality against each key seems to defeat the purpose of using a dict. \$\endgroup\$
    – FMc
    Nov 19 at 18:18
  • 1
    \$\begingroup\$ @FMc yep, that's correct. It was pretty late when I wrote this. Added extra-comments for that part now. \$\endgroup\$ Nov 20 at 10:16
1
\$\begingroup\$

You have a small class where the position must be one of three allowed values and where the bonus is derived purely from the position. Under typical conditions, this implies that you should validate the position and compute the bonus dynamically based on position (don't store the bonus, compute it). The way to achieve both of those things in via properties: a getter for bonus and both a getter and setter for position. Here's an illustration with some additional explanations in the comments.

class Scholar:

    # A data structure that links positions to bonuses and also
    # provides a mechanism to validate the position itself.

    BONUSES = {
        'Professor': 70000,
        'Associate Professor': 50000,
        'Assistance Professor': 30000,   # Shouldn't this be Assistant?
    }

    def __init__(self, name, salary, academic_position):
        self.name = name
        self.salary = salary
        self.academic_position = academic_position

    # Bonus is always computed dynamically, so position and bonus
    # are never out of sync. Since we do not want users of the Scholar
    # class to set the bonus, we only define the getter property.

    @property
    def bonus(self):
        return self.BONUSES[self.academic_position]

    # Position requires actual validation, so we need both a getter and a
    # setter. The setter raises an error if the position is invalid, and the
    # actual attribute is stored under the same name with an underscore prefix,
    # which conveys the idea to class users that the attribute is intended to
    # be managed by the class, not by users.

    @property
    def academic_position(self):
        return self._academic_position

    @academic_position.setter
    def academic_position(self, value):
        if value in self.BONUSES:
            self._academic_position = value
        else:
            # Adjust the error message as needed to explain the problem
            # in more detail or to provide allowed values.
            raise ValueError('Invalid academic_position')

That's the garden-variety way to handle stuff like this, in the sense that you'll find many online tutorials covering approaches like it. One thing to notice is that properties are a bit of a pain: they tend to spawn a lot of boilerplate code, and the whole things starts to feel too much like an unpleasant language that shall not be named.

Before going to this trouble I would first ask a deeper question: should Scholar instance be mutable? Based on what you've told us so far, the answer could be no. In that case, Scholar could be a namedtuple or dataclass. For example:

from collections import namedtuple

Scholar = namedtuple('Scholar', 'name salary academic_position bonus')

BONUSES = {
    'Professor': 70000,
    'Associate Professor': 50000,
    'Assistant Professor': 30000,
}

def make_scholar(name, salary, academic_position):
    # Here you get minimal error handling for free, as a KeyError.
    # If desired, wrap in a try-except structure to raise
    # a more informative ValueError.
    bonus = BONUSES[academic_position]
    return Scholar(name, salary, academic_position, bonus)
\$\endgroup\$
1
  • \$\begingroup\$ Thanks. For your question if it is mutable or not, well, I just started learning OOP in python. the exercise just tells me to create a class Scholar and compute the yearly income which include bonus. so the answer would be.. not concerned! \$\endgroup\$
    – tmo
    Nov 20 at 3:25

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.