I'm trying to access an API using the requests library in as clean a manner as possible. The API is accessing various settings of various devices. I'd like to keep my code as readable and comprehensible as possible, thus I want to avoid setting up my API access functions too abstractly. Therefore, instead of setting_101
I want to edit settings by their associated name.
I reckon a class mapping a the various setting names to their values is the most desirable approach, but given my inexperience with classes I would appreciate some feedback on my attempt.
My ultimate goal is to have a class allowing me to simply access a setting, once an instance is setup, through read.setting_name()
. Previously I simply defined individual functions for every setting which while functional was massively inefficient and a bit of an anti-pattern.
The code below is my attempt at implementing the desired functionality. It works, but it still feels a bit 'rough'.
import requests
class SettingReader:
def __init__(self, domain, headers, device_serial):
self.domain = domain
self.headers = headers
self.device_serial = device_serial
self.setting_ids = {
'setting_A': 1,
'setting_B': 2,
'setting_C': 3,
}
def read_setting_A(self):
return self._read_setting(self.setting_ids['setting_A'])
def read_setting_B(self):
return self._read_setting(self.setting_ids['setting_B'])
def read_setting_C(self):
return self._read_setting(self.setting_ids['setting_C'])
def _read_setting(self, setting_id):
url = f'{self.domain}/device/{self.device_serial}/settings/{setting_id}/read'
response = requests.request('POST', url, headers=self.headers)
return response
domain = "https://my_dummy_api.com"
headers = {"Authorization": "my_token"}
reader = SettingReader(domain, headers, 'my_device_serial')
result_A = reader.read_setting_A()
result_B = reader.read_setting_B()
result_C = reader.read_setting_C()
For these relatively simple query it does give the correct output. I want to be sure of this approach before implementing more complex queries.
print(result_A.json())
print(result_B.json())
print(result_C.json())
> {'data': {'value': val_A}}
> {'data': {'value': val_B}}
> {'data': {'value': val_C}}
While this already feels like a significant improvement (and notably also an easily expandable option for more complex API calls than simple reads) over individually defined functions, I'd rather do it fully right this time around than having to rewrite this code again in the future.
Do you agree a class (and this implementation) is the correct approach for this issue, and are there any changes you'd suggest?
-
1\$\begingroup\$ The code looks like it might be demo code rather than real working code from a project you have written. If it is demo code then the question is off-topic. Please read How do I ask a good question. \$\endgroup\$pacmaninbw– pacmaninbw ♦2024年01月05日 13:23:51 +00:00Commented Jan 5, 2024 at 13:23
-
\$\begingroup\$ This is not demo code, but real working code from a project of mine. I have removed it out its context and given the variables more generic names, thereby only sharing the details relevant for the scope of the question. If it looks like demo code this could be due to my inexperience with Classes generally, as stated. \$\endgroup\$Floriancitt– Floriancitt2024年01月05日 13:28:43 +00:00Commented Jan 5, 2024 at 13:28
-
\$\begingroup\$ Why do your settings have a numeric ID at all? They should be keyed by their unique name as a string. \$\endgroup\$Reinderien– Reinderien2024年01月05日 16:22:57 +00:00Commented Jan 5, 2024 at 16:22
-
\$\begingroup\$ I did not design the API I am connecting too, and am simply using the IDs as defined by the API itself. This is not a public API, which is why the domain/output (which work as intended) are dummies. I'm guessing the original developer opted for numeric IDs due to their quantity, ~200. I too find these numeric IDs insufficiently clear, which is my reason for mapping them to unique names (setting_A, ...) for my purposes. I do not expect the settings associated with the IDs to change, I could foresee more settings being added, though these will likely not be used in my current projects scope. \$\endgroup\$Floriancitt– Floriancitt2024年01月05日 17:06:12 +00:00Commented Jan 5, 2024 at 17:06
-
\$\begingroup\$ We need the context of the code to do a good code review. \$\endgroup\$pacmaninbw– pacmaninbw ♦2024年01月06日 13:19:41 +00:00Commented Jan 6, 2024 at 13:19
2 Answers 2
One can try the approach of creating the methods dynamically. The answer posted by Linny is one way of doing this. Linny's solution implements a __getattr__
that is called when an attribute such as read_setting_A
is undefined when calling reader.read_setting_A()
and then sees if the read_setting_A
is a key in self.settings_id
and if so returns an appropriate lambda function for the missing attribute. If the function should be called again, the read_setting_A
attribute will still be undefined and the __getattr__
method will be invoked again.
Let me suggest two approaches, the second of which is the best if you are willing to access the setting values as read/only properties rather than as method calls:
Approach 1: Create the Class with All Required Methods Up Front
We can define a function create_SettingReader_class
that returns a class definition with all the necessary methods defined based on your settings_ids
dictionary, called SETTINGS_IDS
in the following code:
def create_SettingReader_class():
import requests
import types
SETTINGS_IDS = {
'setting_A': 1,
'setting_B': 2,
'setting_C': 3,
}
# Define the methods we will always have:
def __init__(self, domain, headers, device_serial):
self.domain = domain
self.headers = headers
self.device_serial = device_serial
def _read_setting(self, setting_id):
"""For demo purposes, just return the setting_id argument."""
print('_read_setting called with setting_id =', setting_id, 'domain =', self.domain)
return setting_id
"""
url = f'{self.domain}/device/{self.device_serial}/settings/{setting_id}/read'
response = requests.request('POST', url, headers=self.headers)
return response
"""
def create_read_setting_function(value):
"""Create a method that calls self._read_setting with the specified
value as the argument. Note that this function returns a closure."""
return lambda self: self._read_setting(value)
# Create a dictionary wuth the methods
# we will always have:
cls_dict = {
'__init__': __init__,
'_read_setting': _read_setting
}
# Add to this dictionary dynamically created methods, one for
# each key/value pair in SETTINGS_ID. For example, method
# read_setting_A will be created and it will call _read_setting
# with the setting_id argument set to 1:
cls_dict.update({
'read_' + setting_id: create_read_setting_function(value)
for setting_id, value in SETTINGS_IDS.items()
})
# Return the new class to be assigned to a global variable SettingReader:
return types.new_class('SettingReader', (), {}, lambda ns: ns.update(cls_dict))
SettingReader = create_SettingReader_class()
domain = "https://my_dummy_api.com"
headers = {"Authorization": "my_token"}
reader = SettingReader(domain, headers, 'my_device_serial')
result_A = reader.read_setting_A()
result_B = reader.read_setting_B()
result_C = reader.read_setting_C()
Prints:
_read_setting called with setting_id = 1 domain = https://my_dummy_api.com
_read_setting called with setting_id = 2 domain = https://my_dummy_api.com
_read_setting called with setting_id = 3 domain = https://my_dummy_api.com
A Second Approach: Use Descriptors
Descriptors are very powerful. Properties are implemented as descriptors and we will be creating specialized properties. Now a client will access property setting_A
rather than calling method read_setting_A
. If you do not mind the change of syntax, I believe this is a simpler approach than the previous one:
class SettingProperty:
@staticmethod
def read_setting(setting_id, domain, headers, device_serial):
"""For demo purposes, just return the setting_id argument."""
print('read_setting called with setting_id =', setting_id, 'domain =', domain)
return setting_id
"""
url = f'{domain}/device/{device_serial}/settings/{setting_id}/read'
response = requests.request('POST', url, headers=headers)
return response
"""
def __init__(self, setting_id):
self._setting_id = setting_id
def __set_name__(self, owner, name):
"""We aren't really interested in the name of this property."""
self._name = name
def __get__(self, obj, objtype=None):
return SettingProperty.read_setting(
self._setting_id,
obj.domain,
obj.headers,
obj.device_serial)
class SettingReader:
# Now instead of a dictionary, we have:
setting_A = SettingProperty(1)
setting_B = SettingProperty(2)
setting_C = SettingProperty(3)
def __init__(self, domain, headers, device_serial):
self.domain = domain
self.headers = headers
self.device_serial = device_serial
domain = "https://my_dummy_api.com"
headers = {"Authorization": "my_token"}
reader = SettingReader(domain, headers, 'my_device_serial')
result_A = reader.setting_A
result_B = reader.setting_B
result_C = reader.setting_C
Prints:
read_setting called with setting_id = 1 domain = https://my_dummy_api.com
read_setting called with setting_id = 2 domain = https://my_dummy_api.com
read_setting called with setting_id = 3 domain = https://my_dummy_api.com
Notes
In both approaches when a read-setting method is called or a setting property is accessed, a new request is made to get the value. If this value does not change from call to call, then it could be saved as an attribute in the SettingReader
instance. Naturally you would check to see if the attribute exists before issuing a network request.
This is the modified code for the second approach that caches the setting values:
class SettingProperty:
@staticmethod
def read_setting(setting_id, domain, headers, device_serial):
"""For demo purposes, just return the setting_id argument."""
print('read_setting called with setting_id =', setting_id, 'domain =', domain)
return setting_id
"""
url = f'{domain}/device/{device_serial}/settings/{setting_id}/read'
response = requests.request('POST', url, headers=headers)
return response
"""
def __init__(self, setting_id):
self._setting_id = setting_id
def __set_name__(self, owner, name):
"""Now we are interested in the property's name."""
self._name = name
def __get__(self, obj, objtype=None):
attribute_name = '_' + self._name
value = getattr(obj, attribute_name, None)
if value is None:
# We must go out to the network:
value = SettingProperty.read_setting(
self._setting_id,
obj.domain,
obj.headers,
obj.device_serial)
setattr(obj, attribute_name, value)
return value
class SettingReader:
# Now instead of a dictionary, we have:
setting_A = SettingProperty(1)
setting_B = SettingProperty(2)
setting_C = SettingProperty(3)
def __init__(self, domain, headers, device_serial):
self.domain = domain
self.headers = headers
self.device_serial = device_serial
domain = "https://my_dummy_api.com"
headers = {"Authorization": "my_token"}
reader = SettingReader(domain, headers, 'my_device_serial')
print(reader.setting_A)
print(reader.setting_A)
Prints:
read_setting called with setting_id = 1 domain = https://my_dummy_api.com
1
1
-
\$\begingroup\$ Thank you for the detailed response, learned a great deal from it when you first shared it and implemented many of its design ideas into my project. Since then, my project goals have shifted requiring async requests. My first instinct for this is to have a class similar to your second approach, ensuring the instance gets passed a
httpx.AsyncClient
for the requests, and instead of returning the results returning a coroutine that can be tasked & awaited externally. Do you see any issues with this approach? \$\endgroup\$Floriancitt– Floriancitt2024年02月28日 10:25:00 +00:00Commented Feb 28, 2024 at 10:25 -
1
-
\$\begingroup\$ I've managed to fully implement that and integrate it within my project for the reader class. Thank you! My project (unsurprisingly) also has a
WriteSetting
class, with similar functionality but for writing. I love how clean this approach is, but I don't see how I could transfer it to writing (as__set__
, lacking a return, is not viable here). Is the only option here to have a write setting class with all the setting methods manually defined? \$\endgroup\$Floriancitt– Floriancitt2024年03月01日 15:50:00 +00:00Commented Mar 1, 2024 at 15:50 -
\$\begingroup\$ Sorry - I am not understanding what you are saying. Are you saying that because the descriptor
__set__
method does not return a value, you cannot use a descriptor? If so, why would you need for it to return a value? \$\endgroup\$Booboo– Booboo2024年03月01日 18:42:52 +00:00Commented Mar 1, 2024 at 18:42 -
1\$\begingroup\$ I think it is time to post your code as a new code review request. Besides being a new issue, you would have a larger audience of potential reviewers. If you do create a new post, you can respond with a comment here to the new link. \$\endgroup\$Booboo– Booboo2024年03月03日 17:13:03 +00:00Commented Mar 3, 2024 at 17:13
Dynamic Method Creating
Instead of creating a method for each setting, you can dynamically create methods based on the setting names. This makes it a lot easier to call specific settings without having to write these methods yourself, while also making it more scalable and easier to maintain.
def __getattr__(self, name):
prefix = 'read_setting_'
if name.startswith(prefix):
setting_name = name[len(prefix):]
if setting_name in self.setting_ids:
return lambda: self._read_setting(self.setting_ids[setting_name])
raise AttributeError(f"'SettingReader' object has no attribute '{name}'")
Error Handling
Right now, if you pass in an invalid request, everything fails horribly. Add a simple raise
to help the user know what went wrong.
if setting_id is not None:
return lambda: self._read_setting(setting_id)
else:
raise ValueError(f"Setting '{setting_name}' not found.")
Explore related questions
See similar questions with these tags.