2
\$\begingroup\$

Please review my code where I want to handle the timeout errors and send parsed JSON response if any. There are times when a POST request does not ALWAYS return JSON code.

def use_service(branch: str, relative_url: str, request_type: str = 'GET', data=None, retry=1):
    """
    :param – relative_url – the relative url to be submitted
    :param – request_type – the type of request being submitted (POST, GET, PUT, etc.)
    :param – data – the data to be submitted with the request if necessary.
    """
    logger.info(f'getting data from {relative_url}')
    url = envs.get(branch, '') + relative_url
    resp = {}
    status = False

    for x in range(retry):
        try:
            if request_type == 'GET':
                print(f'GET call to {url}')
                resp = requests.get(url)
            elif request_type == "POST":
                print(f'POST call to {url}')
                if branch in envs.keys():
                    resp = requests.post(
                        url,
                        json=data,
                    )
                else:
                    print(f'{branch} : Attempted to push data: {data}')
            status = resp.ok
            if resp.ok:
                resp = resp.json()
                print(f'{resp}')
            else:
                resp.raise_for_status()
        except HTTPError as err:
            logger.exception(f'HTTP Error: {err}')
        except Timeout as err:
            logger.exception(f'Connection timed out {err}')
        except ConnectionError:
            logger.exception(f'A network problem has occured (DNS failure, refused connection)')
        except RequestException as err:
            logger.exception(f'Other error occurred: {err}')
        except Exception as err:
            logger.exception(f'Unknown Exception: {err}')

    if not status:
        resp = []

    return resp
\$\endgroup\$
  • \$\begingroup\$ @AlexV Thanks for pointing it out. I have edited the code to be fully working. \$\endgroup\$ – technazi Nov 4 at 7:42
  • \$\begingroup\$ @technazi, POST request does not ALWAYS return JSON code - what error is caught on that event? \$\endgroup\$ – RomanPerekhrest Nov 4 at 7:47
  • \$\begingroup\$ The final exception block should catch it. \$\endgroup\$ – technazi Nov 4 at 7:54
3
\$\begingroup\$

General Remarks

  • Avoid catching Exception, catch a specific exception instead to avoid silencing errors.
  • Don't return the empty list as an indication of failure. Either return None or (preferably, in case of failure) raise an exception.
  • It's nice to see you adopt typing as an important part of your code. I recommend being more consistent with it in places where you already use it, and using it even more. For example, two of the parameters in your function were not annotated, neither was the return value. Use type aliases to describe non-trivial types, such as input and output data of your requests (i.e., type of data and return type). A final example I'll show in the code is the use of enums instead of strings, which can prevent you from making mistakes.
  • I'm not going to say the function is "too complex". Yet. But if it grows to handle more HTTP methods (say, PUT, HEAD, ...), or the handling of the existing HTTP methods grows, you should seriously consider decomposing it into multiple functions or by incorporating object-oriented design.
  • Good job on the documentation attempt, but try to follow a set style in the future. Examples include PEP 287, NumPy, Google. A general guideline is given in PEP 257. This includes advantages such as consistency and automated generation of HTML docs using e.g. sphinx-doc.
  • Some of the control flow is a bit confusing. I've made edits to improve it (see below) and noted some explanations of the issues. I generally recommend you to investigate these further while writing the code.
  • I highly recommend you to read a style guide and stick to it, if you haven't already. I also suggest installing a linter so that you can be warned of dubious code style and possible bugs while writing the code. I'm fairly sure a linter would've complained about quite a bit of the remarks I'll give below. I'll give a concrete recommendation for a linter at the end of this answer.

Further recommended reads:

  • The Zen of Python (PEP 20).
  • PEP8 style guide (PEP 8), although you can occasionally find me diverging from the guidelines.

To apply a couple of quotes from PEP 20:

Errors should never pass silently.

Your except Exception as err: guard would catch most of the exceptions that exist in Python, and thus also catches a potential AttributeError that may occur in your code. See my comments in the code for more details.

In the face of ambiguity, refuse the temptation to guess.

I've had to guess multiple times to come to a purpose for some parameters, either because non-descript names, or because of missing type information. E.g., without looking at the function body, can I know whether I can pass a list as the data parameter? What is the branch parameter?

Personal Remarks

A couple of personal pet peeves related to your code:

  • Work on your variable names. I personally dislike names like data or resp since they don't really give much information. Similarly, the retry parameter is a bit misleading. Ideally, variable names should give me all the information required to understand their purpose (although this is often not possible when keeping them brief). Additionally, I'd like better names for err, but I name my caught exceptions exc too often so I'm not in a position to critique that :)
  • Don't spam the user with info logs that should be debug logs. The user doesn't care that your function is doing this or that. I suspect these may be debugging statements (use a debugger!), so use logger.debug so you don't have to change it later. Definitely never use print, since disabling that is non-trivial.

I've left a bunch of comments in your code while I was reading through it, and I've also rewritten a couple snippets to illustrate the difference.

"""Modules should always start with a docstring describing their exports.

Since this is just a snippet of one function, I'm assuming your module has one.
If not, write one.
"""
from typing import Any, Optional

import enum
import os
from json.decoder import JSONDecodeError

import requests
from requests.exceptions import HTTPError, RequestException, Timeout


# COMMENT: If I left these here, ignore these, they are just to make the code
# run.
class Logger:
    """Logging stub for review."""

    def debug(self, msg: str) -> None:
        print(msg)

    def info(self, msg: str) -> None:
        print(msg)

    def warn(self, msg: str) -> None:
        print(msg)

    def exception(self, msg: str) -> None:
        print(msg)


logger = Logger()
envs = os.environ


# COMMENT: Prefer custom data types for enumerated data instead of simple
# string literals such as 'get', 'post', ... This way, mistakes can be caught
# early by static analysis tools such as mypy, e.g. misspelling 'post' as
# 'psot'.
class RequestMethod(enum.Enum):
    """Enumeration of various request methods."""

    GET = 1
    POST = 2
    # PUT, DELETE, OPTION, HEAD, ...


ResponseType = Any  # TODO: Update the response type.


# COMMENT: PEP8 dictates line length to be limited to 79 characters. I,
# personally, feel like this is too short, yet this serves as an example of a
# possible method of breaking up your function signatures in case of overly
# long lines. This allows you to specify your types and default values so that
# they stand out immediately. Parameters are indented at two increments in
# levels, and the return type indented at the same level as the function name.
# This way, you can not confuse the return type signature, the parameter list,
# or the body of the function.
def use_service(
        branch: str,
        relative_url: str,
        # COMMENT: Nitpicky, but the correct terminology is "request method",
        # not "request type": https://tools.ietf.org/html/rfc7231#section-4
        # Both are pretty much used interchangeably though.
        request_method: RequestMethod = RequestMethod.GET,
        # COMMENT: Try to annotate all of your parameters, even those with
        # default values. Especially for `data`, which has a default value of
        # `None`, a type hint can be useful documentation. Additionally, mypy,
        # the state-of-the-art static type checker for Python, is sometimes
        # unable the parameter type, even if there are default values.
        # COMMENT: Give your parameters descriptive names. `data` is too
        # generic of a word. `retry` is less generic, and from just looking at
        # the name, I can get an idea of its purpose, but explicitly calling it
        # "number of tries" makes it immediately clear what the full purpose
        # is without reading the documentation.
        post_data: Optional[Any] = None,
        num_tries: int = 1
        # Always document your return types. mypy WILL complain about it if run
        # in strict mode.
) -> Optional[ResponseType]:
    """Perform a request and retry if it failed.

    Perform a request of type ``request_type`` to the given ``relative_url``
    and return the parsed JSON response. If the request fails for any reason,
    retry it until ``num_tries`` tries have been performed. For ``POST``
    requests, the sent data is to be supplied by the ``post_data`` parameter.

    .. todo:: Document the purpose of the ``branch`` parameter.

    :param branch: ???
    :param relative_url: The relative URL to be submitted.
    :param request_type: The type of request being submitted.
    :param post_data: The data to be submitted with the request, if necessary.
    :param num_tries: The number of times the request should be tried before
                      giving up.
    :returns: The parsed JSON response, or `None` if the request failed after
              all tried attempts.

    """
    # COMMENT: Documentation is often overlooked but vital, both for you as
    # well as your colleagues. In my opinion, good documentation for a function
    # contains three things:
    #   1. A brief description of what the function does.
    #   2. An overview of the parameters I should/can give it, as well as their
    #      purpose.
    #   3. A description of what I can expect to receive from the function as
    #      a return value.
    # See PEP257 for docstring conventions.
    # I've rewritten the documentation into a more machine-readable format,
    # reST. It can be parsed and used to generate HTML documentation by tools
    # such as sphinx-doc. Very useful when writing an API.
    # Documentation often also includes type information on the parameters and
    # the return type. This is vital. However, I've emitted this from the
    # docstring since such information is already available as type hints.
    # Sphinx has plugins to extract type information from type hints.
    # COMMENT: This should really be a debug statement rather than info. Also,
    # I'm assuming the logger is imported/constructed somewhere outside of the
    # snippet provided.
    logger.debug(f'getting data from {relative_url}')

    # COMMENT: What is `envs`? If it's just accessing environment variables,
    # consider using `os.environ` instead, especially if its a third-party
    # package.
    # COMMENT: The `url` variable can get a name improvement, IMO.
    url = envs.get(branch, '') + relative_url
    # COMMENT: These variables are now useless. They could've also used a
    # better name.
    # resp = {}
    # status = False

    # COMMENT: Although this loop works, it would ALWAYS retry, regardless of
    # there being any exceptions. If the request was fine, it would do it again
    # if `num_tries` is high enough. I've renamed the parameter to `num_tries`
    # instead of `num_retries` so that a `num_tries` value of 2 will try twice.
    # For a parameter named `num_retries` given value 2, I would expect the
    # request to be performed at most 3 times: Once initially, and two REtries.
    # COMMENT: We don't use variable `x` anywhere in the loop. Use an
    # underscore instead so that it is not tied to a variable.
    for _ in range(num_tries):
        try:
            # COMMENT: Because we use an enum, we can now compare literally,
            # which is more efficient than string comparisons and decreases
            # the change of misspellings.
            # COMMENT: Since these handlers are short, I'll allow it, but if
            # you get more options and longer code, consider using classes
            # and polymorphism. E.g.:
            # req_maker = some_way_of_obtaining_an_object(req_method)
            # req_maker.perform(url)
            if request_method is RequestMethod.GET:
                # COMMENT: You changed these to prints in the edit, I changed
                # them back to logger calls. As a rule of thumb: Use a logger,
                # never print. Exceptions should be rare and only for small
                # personal scripts. I also changed them to debug logs, since
                # they're not informative to the user.
                logger.debug(f'GET call to {url}')
                # COMMENT: `resp` can get a better name, more descriptive of
                # its purpose. I'm assuming here you're contacting a REST API.
                api_resp = requests.get(url)
            elif request_method is RequestMethod.POST:
                logger.debug(f'POST call to {url}')
                if branch in envs.keys():
                    # COMMENT: In cases like these, indent your arguments with
                    # two levels, so that you don't confuse it for a new block
                    # in the body. Also, there's plenty of controversy over
                    # where to put the closing parenthesis in these cases,
                    # and whether or not to allow a comma after the final item.
                    # I'm personally not a fan, but that's just me.
                    api_resp = requests.post(
                            url,
                            json=post_data,
                    )
                else:
                    # COMMENT: Some style guides disallow the usage of string
                    # interpolation, I don't personally mind it.
                    logger.warn(
                            f'{branch} : Attempted to push data: {post_data}')
                    # COMMENT: You should really do *something* other than just
                    # logging here. In your original code, here, `resp` would
                    # not get defined, and thus the next line (`resp.ok`) would
                    # fail with an `AttributeError`, which would get caught as
                    # an "Unknown exception", and potentially retry the loop
                    # because of this bug.
                    # It seems like this is an unrecoverable problem with the
                    # input, so it's better to just return and stop the rest
                    # of the evaluation of the function. Even better: Raise an
                    # exception.
                    return None

            # COMMENT: I'm going to completely rewrite this part.
            # status = api_resp.ok
            # if api_resp.ok:
            #     resp = api_resp.json()
            #     print(f'{resp}')
            # else:
            #     resp.raise_for_status()

            # `raise_for_status` raises an exception if status != 2xx.
            # In your original code, you first check the status, and if the
            # status != 2xx, you call `raise_for_status`, which itself checks
            # if status != 2xx and raises. Why?
            api_resp.raise_for_status()
            # This may throw an exception. As you mention, the API "does not
            # ALWAYS return JSON", so you should expect exceptions. You should
            # expect parsing errors anyway if relying on external data.
            json_resp = api_resp.json()
            logger.debug(json_resp)
            # Return the data early instead of assigning to a variable
            # returning it at the end of the function. This breaks free from
            # the loop, thus immediately fixing the issue with too many tries
            # in the loop.
            return json_resp
        # Let's get to the exception handling...
        # COMMENT: It should come as no surprise now, I don't like `err`.
        # Either use something like `http_err` or just simply `e`, the standard
        # convention.
        except HTTPError as err:
            logger.exception(f'HTTP Error: {err}')
        except Timeout as err:
            logger.exception(f'Connection timed out {err}')
        except ConnectionError:
            # COMMENT: Don't use the `f` prefix if you're not interpolating.
            # COMMENT: Split the string for line length. Remember that this
            # works without requiring a `+`.
            logger.exception(
                    'A network problem has occured '
                    '(DNS failure, refused connection)')
        except RequestException as err:
            logger.exception(f'Other error occurred: {err}')
        # COMMENT: Don't catch Exception. Many style guides will discourage you
        # from doing this. `Exception` covers most of the possible exceptions
        # in Python programs, including `AttributeError`, which caused the bug
        # in the `else` branch of the POST request handler.
        # except Exception as err:
        #     logger.exception(f'Unknown Exception: {err}')

        # COMMENT: Explicitly catch JSONDecodeError so you can handle JSON
        # parsing errors explicitly.
        except JSONDecodeError as e:
            logger.exception(f'Failed to parse JSON: {e}')

    # COMMENT: When you have nothing meaningful to return from a function,
    # either don't return anything, or explicitly return `None`. Returning the
    # empty list is confusing: What if the response JSON was actually an empty
    # list? We'd have no way of discriminating a failure from an empty
    # response!
    # Better yet, we should raise an exception if all tries failed.
    return None

Wrapping Up

The improvements I've proposed definitely leave a lot of room for further improvement. Especially if you're planning on extending the functionality shown (which, likely, you will or even have to, given the nature of the code), I highly recommend to start decomposing now. For example, extract handling code for each of the HTTP methods into its own individual function, and perform some efficient dispatching by either putting the functions in a dict with keys corresponding to the HTTP methods, or implementing it using OO and polymorphism. The former is likely the better approach for this code, opting for the latter is a good idea if you plan on adding lots of functionality to these requests.

Bonus: Linter

If you want my recommendation for a Python linter, I run flake8 on my personal code with the following plugins:

flake8 --version
3.7.9 (aaa: 0.6.2, cohesion: 0.9.1, flake-mutable: 1.2.0, flake8-annotations-complexity: 0.0.2, flake8-annotations-coverage: 0.0.3, flake8-bandit: 2.1.2, flake8-blind-except: 0.1.1, flake8-broken-line: 0.1.1, flake8-bugbear: 19.8.0, flake8-comprehensions: <cached_property.cached_property object at 0x10e93d350>, flake8-docstrings: 1.5.0, pydocstyle: 4.0.1, flake8-eradicate: 0.2.3, flake8-pie: 0.2.2, flake8-print: 3.1.4, flake8-pyi: 19.3.0, flake8-string-format: 0.2.3, flake8-tidy-imports: 2.0.0, flake8-todo: 0.7, flake8-tuple: 0.4.0, flake8-variables-names: 0.0.1, flake8_builtins: 1.4.1, flake8_pep3101: 1.2.1, flake8_quotes: 2.1.0, import-order: 0.18.1, logging-format: 0.6.0, mccabe: 0.6.1, naming: 0.8.2, pycodestyle: 2.5.0, pyflakes: 2.1.1, rst-docstrings: 0.0.10, warn-symbols: 1.1.1) CPython 3.7.5 on Darwin

But there's a bunch of warnings that I tend to disable here as I see fit. Another option is wemake-python-styleguide, which is a very strict plugin for flake8 (too strict for me, even). mypy is also great as a static type checker to find type bugs early.

\$\endgroup\$

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