1
\$\begingroup\$

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

    @staticmethod
    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,
                                timeout=timeout)
        return response

    @staticmethod
    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,
                                 timeout=timeout)
        return response

    @staticmethod
    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,
                                timeout=timeout)
        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,
                                    timeout=timeout)
        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
        try:
            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)
            else:
                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

    @staticmethod
    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
\$\endgroup\$
2
  • \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 3 → 2. \$\endgroup\$ 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
    Dec 3, 2018 at 22:32

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Agree. But see the response handler method make the code clean. Second observe the fallback method \$\endgroup\$ 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\$ 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\$ Dec 3, 2018 at 3:56
  • \$\begingroup\$ @DineshSaini Edited. Most of your code needs to go away. \$\endgroup\$
    – Reinderien
    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\$ Dec 5, 2018 at 5:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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