1
\$\begingroup\$

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?

asked Jan 5, 2024 at 12:05
\$\endgroup\$
5
  • 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\$ Commented 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\$ Commented 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\$ Commented 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\$ Commented Jan 5, 2024 at 17:06
  • \$\begingroup\$ We need the context of the code to do a good code review. \$\endgroup\$ Commented Jan 6, 2024 at 13:19

2 Answers 2

2
\$\begingroup\$

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
answered Jan 10, 2024 at 21:39
\$\endgroup\$
7
  • \$\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\$ Commented Feb 28, 2024 at 10:25
  • 1
    \$\begingroup\$ Sounds feasible -- perhaps something like this? \$\endgroup\$ Commented Feb 28, 2024 at 13:14
  • \$\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\$ Commented 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\$ Commented 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\$ Commented Mar 3, 2024 at 17:13
2
\$\begingroup\$

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.")
answered Jan 5, 2024 at 20:51
\$\endgroup\$
0

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.