This is a wrapper that can be used to do additional external REST call to some API. Since I have to provide support to legacy code so I added three static functions to support them.

However, the question that comes to my mind is where to actually set default value either in the constructor or it should be in the main function.

is_optional = kwargs.get('is_optional', False) can also be set directly in constructor. So what is correct way to handle such situations?

class ExternalRestService:
    def __init__(self, endpoint, method=sc.REQUEST_GET):
        self.endpoint = endpoint
        self.method = method
        self.timeout = sc.REQUEST_TIMEOUT

    def get(endpoint, request_headers, timeout):
          Handle GET External Call
        logging.debug("About to make an external *** GET *** to url %s", endpoint)
        response = requests.get(url=endpoint, headers=request_headers,
        return response

    def post(endpoint, request_headers, json_data, timeout):
          Handle POST External Call
        logging.debug("About to make an external *** POST *** to url %s", endpoint)
        request_headers["Content-type"] = "application/json"
        response = requests.post(url=endpoint, headers=request_headers, json_data=json_data,
        return response

    def put(endpoint, request_headers, json_data, timeout, fallback=False):
          Handle PUT External Call
        logging.debug("About to make an external *** PUT *** to url %s", endpoint)
        request_headers["Content-type"] = "application/json"
        response = requests.put(url=endpoint, headers=request_headers, json_data=json_data,
        if fallback and response.status_code == 405:
            logging.debug("PUT action failed, applying fallback strategy")
            response = requests.put(url=endpoint, headers=request_headers, json_data=json_data,
        return response

    def invoke(self, **kwargs):
        Execute the external call
        is_fallback = kwargs.get('is_fallback', False)
        is_optional = kwargs.get('is_optional', False)
        request_headers = kwargs.get('request_headers', {})
        json_data = kwargs.get('json_data', {})

        response_list = None
            logging.info("About to make an external *** %s *** call", self.method)
            if self.method == sc.REQUEST_POST:
                response = self.post(self.endpoint, request_headers, json_data, self.timeout)
            elif self.method == sc.REQUEST_PUT:
                response = self.put(self.endpoint, request_headers, json_data,
                                    self.timeout, fallback=is_fallback)
                response = self.get(self.endpoint, request_headers, self.timeout)
            response_list, error_list = self.__response_handler(response, is_optional)
        except requests.RequestException as err:
            error_list = err
        return response_list, error_list

    def __response_handler(response, is_optional=False):
       Handle API response
        status_code = response.status_code
        if is_optional and status_code == 404:
            return response, None
        if not 200 <= status_code < 300:
            return None, response
        response_list = response.json()
        return response_list, None
  • \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 3 → 2. \$\endgroup\$ Commented Dec 3, 2018 at 21:52
  • \$\begingroup\$ I have reviewed this question, and agree that the edits invalidated the current answer. Since the bounty was posted on the edited question, I have refunded the bounty. Please consider asking a new question and apply the bounty to that one if you want the revised code reviewed. \$\endgroup\$
    – rolfl
    Commented Dec 3, 2018 at 22:32

1 Answer 1


Overall the existence of this code does not seem justified. It seems like boiler paste that doesn't offer a whole lot of new functionality, and people are better off calling requests directly.

It also doesn't make sense as a class. There's enough static content that it should be a module, not a class. It's good that you're thinking about factoring the fallback feature into a common module, but you need to take a different approach.

One thing you can do is make a subclass of requests.Session, but even that doesn't really seem necessary. Things to avoid:

  • Don't add logging for stuff that's already logged by requests.
  • Pass args and kwargs implicitly instead of explicitly copying stuff.
  • Explicitly pass is_fallback instead of passing it implicitly in kwargs.
  • Just return the response. Don't convert it to JSON. It's like seven more characters at the calling side, and simplifies everything.
  • Don't rename request to invoke. Just call it request.
  • Don't rename url to endpoint. Just call it url.
  • Don't rename json to json_data. Don't even pass it explicitly. It's quite simple to pass it as json in the kwargs.
  • Don't set the content type as you are. When requests sees the json kwarg it will do this for you.

In my opinion, at the absolute most, you should condense this to one utility method:

import logging
import requests

def request(method, url, is_fallback=False, **kwargs):
    response = requests.request(method, url, **kwargs)
    if is_fallback and response.status_code == 405:
        logging.debug('%s action failed, applying fallback strategy', method)
        return requests.request(method, url, **kwargs)
    return response

Anything else is unnecessary boilerplate.

  • \$\begingroup\$ Agree. But see the response handler method make the code clean. Second observe the fallback method \$\endgroup\$ Commented Dec 2, 2018 at 19:25
  • \$\begingroup\$ Also see invoke method and use of is optional and other things. It is used at multiple place so optimize I used class here so that it can be called with object and static method cretqe to support legacy \$\endgroup\$ Commented Dec 2, 2018 at 19:27
  • \$\begingroup\$ Is optional and fallback is just custom thing to do. If I call directly there then I have to add many if and else. Can you give a brief why it should be only module not class. \$\endgroup\$ Commented Dec 3, 2018 at 3:56
  • \$\begingroup\$ @DineshSaini Edited. Most of your code needs to go away. \$\endgroup\$
    – Reinderien
    Commented Dec 4, 2018 at 19:49
  • \$\begingroup\$ How about is_optional because it is used do we need to through error or just surpass the error and other code does not stop even on throw error. \$\endgroup\$ Commented Dec 5, 2018 at 5:34

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.