1

I'm writing some code for library, but I'm not sure, whether it is efficiency or not. I should set variables like roll, pitch, yaw, throttle. do I have to write all property and setter? is it best way?

@property
def roll(self):
    return self._roll

@roll.setter
def roll(self, value):
    if(not isinstance(value, int)):
        print("You should put the integer number!")
        value = int(value)

    if value > 100:
        value = 100
    elif value < -100:
        value = -100

    self._roll = value
Amily
  • 325
  • 1
  • 4
  • 16
  • Using `property` and `setter` is less about efficiency and more about stability and security. By making every access go through a method, you can do things like validation, authorisation etc. It's also easier to debug and test methods. Some good reasons to uyse them are given [here](https://stackoverflow.com/questions/1568091/why-use-getters-and-setters-accessors/1568230#1568230) – match Jan 18 '18 at 21:15

1 Answers1

2

I'm writing some code for library, but I'm not sure, whether it is efficiency or not. I should set variables like roll, pitch, yaw, throttle. do I have to write all property and setter? is it best way?

First off, be careful with trying to optimize your code too early. More often than not what actually happens when someone attempts to optimize their code, is that they introduces unnecessary bugs into their program. This is not to say that you should never optimize your code. When their is clearly room to restructure your code for efficiency, do so. But avoid trying to be overly 'clever' and blindly 'optimizing' things that aren't apparent.

With all that said, your code looks fine. You're using property exactly as it should be used; To do extra, necessary work during getting and settings of attributes (often more so with setting), without the verbosity of forcing users of your class to use method calls. Don't concern yourself to much with efficiency. As long as your not doing something blatantly inefficient, your fine. As @match said in the comments, using property is more about providing maintainability, stability, and readability, than efficiency.

However, there are severals small areas where you're code could be improved. Namely your type and bounds checking. You can extract this into a separate method and call it where needed:

def _check_value(value):
    try:
        value = int(value)
    except ValueError as err:
        raise ValueError('only integer values are permitted') from err

    if value > 100:
        return 100
    elif value < -100:
        return -100
    return value

@property
def roll(self):
    return self._roll

@roll.setter
def roll(self, value):
    self._roll = self._check_value(value)

@property
def pitch(self):
    return self._pitch

@pitch.setter
def pitch(self, value):
    self._pitch = self._check_value(value)
Christian Dean
  • 22,138
  • 7
  • 54
  • 87
  • Also, the isinstance check is going to be more error prone than you might think. It would be better (and more pythonic) to just convert value to an int before returning it. (`return int(value)`) – Beefster Jan 18 '18 at 22:33
  • @Beefster Thanks for raising that concern. Unfortunately, that wouldn't necessarily work, since the OP wants to display a custom error message to the user. However, looking back at my code, you are indeed correct that using `isinstance` would be error prone. So instead I'll use a `try/except` block which is idiomatic. – Christian Dean Jan 18 '18 at 22:46