CODEWITHSUNDEEP
pythonpython-2.7python-3.x
Qwerty
Qwerty

Reputation: 778

Best practice for "get" functions

I am new to Python.

Assume I have a dictionary which holds power supply admin state. (OK = Turned on. FAIL = Turned off).

There are several way to write the "get" function:

1st way

is_power_supply_off(dictionary)
  gets the admin state from dictionary.
  returns true if turned off.
  returns false if turned on.

is_power_supply_on(dictionary)
  gets the admin state from dictionary.
  returns true if turned on.
  returns false if turned off.

2nd way

is_power_supply_on_or_off(dictionary, on_or_off)
  gets the admin state from dictionary.
  returns true/false based on the received argument

3rd way

get_power_supply_admin_state(dictionary)
  gets the admin state from dictionary.
  return value.

Then, I can ask in the function which calls the get function

if get_power_supply_admin_state() == turned_on/turned_off...

My questions are:

Which of the above is considered best practice?

If all three ways are OK, and it`s just a matter of style, please let me know.

Is 1st way considered as "code duplication"? I am asking this because I can combine the two functions to be just one (by adding an argument, as I did in the 2nd way. Still, IMO, 1st way is more readable than 2nd way.

I will appreciate if you can share your thoughts on EACH of the ways I specified.

Thanks in advance!

Upvotes: 3

Views: 187

Answers (5)

Łukasz Rogalski
Łukasz Rogalski

Reputation: 23233

result = is_power_supply_off(state)
result = is_power_supply_on(state)
result = not is_power_supply_on(state)  # alternatively, two functions are certainly not needed

I strongly prefer this kind of naming for sake of readability. Let's just consider alternatives, not in function definition but where function is used.

result = is_power_supply_on_or_off(state, True)
    pass
result = is_power_supply_on_or_off(state, False)
    pass
if get_power_supply_admin_state(state):
    pass
if not get_power_supply_admin_state(state):
    pass

All of these codes requires map of what True and False means in this context. And to be honest, is not that clear. In many embedded systems 0 means truthy value. What if this function analyses output from system command? 0 (falsy) value is indicator of correct state/execution. In a result, intuitive True means OK is not always valid. Therefore I strongly advice for first option - precisely named function.

Obviously, you'll have some kind of private function like _get_power_supply_state_value(). Both function will call it and manipulate it's output. But point is - it will be hidden inside a module which knows what means what considering power supply state. Is implementation detail and API users does not need to know it.

Upvotes: 0

TheBlackCat
TheBlackCat

Reputation: 10328

I would say that the best approach would be to have only a is_power_supply_on function. Then, to test if it is off, you can do not is_power_supply_on(dictionary).

This could even be a lambda (assuming state is the key of the admin state)::

is_power_supply_on = lambda mydict: mydict['state'].lower() == 'ok'

The problem with the first approach is that, as you say, it wastes codes.

The problem with the second approach is that, at best, you save two characters compared to not (if you use 0 or 1 for on_or_off), and if you use a more idiomatic approach (like on=True or on_or_off="off") you end up using more characters. Further, it results in slower and more complicated code since you need to do anif` test.

The problem with the third approach is in most cases you will also likely be wasting characters compared to just getting the dict value by key manually.

Upvotes: 2

Brent Washburne
Brent Washburne

Reputation: 13158

It's more Pythonic to store the dictionary in a class:

class PowerSupply(object):
    def __init__(self):
        self.state = {'admin': 'FAIL'}

    def turn_on(self):
        self.state['admin'] = 'OK'

    def is_on(self):
        return self.state['admin'] == 'OK'

(add more methods as needed)

Then you can use it like this:

ps = PowerSupply()
if not ps.is_on():
    # send an alert!

Upvotes: 0

SuperBiasedMan
SuperBiasedMan

Reputation: 9969

The general Pythonic style is to not repeat yourself unnecessarily, so definitely the first method seems pointless because it's actually confusing to follow (you need to notice whether it's on or off)

I'd gravitate most to

get_power_supply_admin_state(dictionary)
  gets the admin state from dictionary
  return value

And, if I'm reading this correctly, you could go even further.

power_supply_on(dictionary)
  return the admin state from dictionary == turned on

This will evaluate to True if it's on and False otherwise, creating the simplest test because then you can run

if power_supply_on(dictionary):

Upvotes: 0

Spirine
Spirine

Reputation: 1877

Even if this solution isn't in your propositions, I think the most pythonic way of creating getters is to use properties. As it, you'll be able to know whether the power supply is on or off, but the user will use this property as it was a simple class member:

@property
def state(self):
    # Here, get whether the power supply is on or off
    # and put it in value
    return value

Also, you could create two class constants, PowerSupply.on = True and PowerSupply.off = False, which would make the code easier to understand

Upvotes: 0

Related Questions