0
\$\begingroup\$

We would like for a code review for our Python code. The code is written in Python 3.7 and is part of a class handling HTTP cookies.

The code:

class CookieJar(object):
    def add_cookie_header(self, request):
        w_r = WrappedRequest(request)
        self.policy._now = self.jar._now = int(time.time())

        # the cookiejar implementation iterates through all domains
        # instead we restrict to potential matches on the domain
        req_host = request.hostname
        if not req_host:
            return

        if not IPV4_RE.search(req_host):
            hosts = potential_domain_matches(req_host)
            if '.' not in req_host:
                l_req_host = "%s%s" % (req_host, '.local')
                hosts += [l_req_host]
        else:
            hosts = [req_host]

        Cookies = []
        for hosti in range(len(hosts)):
            if hosts[hosti] in self.jar._cookies:
                Cookies.extend(self.jar._cookies_for_domain(hosts[hosti], w_r))

        ats = self.jar._cookie_attrs(Cookies)
        if ats:
            if w_r.has_header("Cookie") == False:
                w_r.add_unredirected_header("Cookie", "; ".join(ats))

        self.processed = self.processed + 1
        if self.processed % self.check_expired_frequency == 0:
            # This is still quite inefficient for large number of Cookies
            self.jar.clear_expired_cookies()

The class CookieJar has other fields and methods, obviously, but they shouldn't be reviewed, so they're not provided here. You can assume all required imports are present at the top of the file and all referenced methods and fields do exist.

How can we make our code more elegant, efficient and maintainable?

\$\endgroup\$

closed as off-topic by Graipher, esote, Mast, Edward, Grajdeanu Alex. Apr 19 at 13:50

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Graipher, esote, Mast, Edward, Grajdeanu Alex.
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ While you claim that all needed fields exits, you don't say what type they are. This can make all the difference, e.g. in a call like if hosts[hosti] in self.jar._cookies. in general we prefer to have as complete a piece of code as possible, for this exact reason. \$\endgroup\$ – Graipher Apr 17 at 9:32
  • \$\begingroup\$ "You can assume" No no, assumptions are the root of all miscommunication. Please supply the imports and all referenced methods. There's a character limit of over 65k characters on Code Review questions, don't be afraid to use them. \$\endgroup\$ – Mast Apr 18 at 0:44
4
\$\begingroup\$

Since you are not showing a whole lot of the class itself, there is not to much fundamental to be done on the presented code.

Documentation

First, you should add documentation to your class and to its methods. Use the """...""" to write those docstrings as recommended in the official Style Guide.

Style

The Style Guide also has a section on variable naming (snake_case is recommended). You're mostly following this convention, and that's also the reason why Cookies as variable name almost immediately caught my attention while first screening the code. The widely accepted convention is to only use names with the first letter capitalized for class names.

Looping

While I was at it, the following piece of code caught my attention next.

Cookies = []
for hosti in range(len(hosts)):
    if hosts[hosti] in self.jar._cookies:
        Cookies.extend(self.jar._cookies_for_domain(hosts[hosti], w_r))

Since hosts is a list, and you're not trying to do anything fancy on the elements themselves, this can be simplified to

cookies = []
for host in hosts:
    if host in self.jar._cookies:
        cookies.extend(self.jar._cookies_for_domain(host, w_r))

which I find easier to read.

String formatting

This snippet comes a little bit earlier in the code: l_req_host = "%s%s" % (req_host, '.local'). It uses the old-old Python string formatting using %. Since you specifically tagged your question as Python 3, I strongly recommend to make use of the new f-string synthax which allows you to write that same line as l_req_host = f"{req_host}.local". Here is a nice blog post talking about all the different ways of string formatting in Python and their pros and cons.

if statements

Next up on my list would be if w_r.has_header("Cookie") == False. In general there is no need to do if sth == True or if sth == False in Python. Both can be conveniently expressed as if sth or if not sth. Usually this will also allow someone to read the expression almost as if it was text. If that doesn't work, maybe think about the name of your bool variable.

Minor tweaks

At the moment the last thing I could come up with was to change self.processed = self.processed + 1 to self.processed += + 1 which saves you a few characters of typing in the future.

\$\endgroup\$

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