7
\$\begingroup\$

This is a problem from Automate the Boring Stuff from "Pattern Matching with Regular Expression". I am a beginner in Python and the purpose of this program is to match urls which start with http:// or https://, with some urls having an optional www. after https://

How can this code be improved?

#! /usr/bin/python3
#websiteURL.py - Find website URLs that begin with http:// or https://

import pyperclip, re

#protocol Regex - checks for http:// or https://
protocolRegex = re.compile(r'''
    https?://           #match http:// or https://
    (?:w{3}\.)?         #www-dot
    [a-zA-Z0-9_-]+      #domin name
    \.                  #dot
    [a-zA-Z]{2,3}       #extension
    ''', re.VERBOSE)

text = str(pyperclip.paste()) #copying data from document to clipboard and converting it into a string
matches = [] #stores all matches in this list

for website in protocolRegex.findall(text): #finding website from the string text
    matches.append(website)

if len(matches) > 0:
    pyperclip.copy('\n'.join(map(str, matches))) #copying result to clipboard after adding newline after each match
    print('Copied to clipboard:')
    print('\n'.join(matches))
else:
    print('No website found')

Running code:

chmod +x websiteURL.py
./websiteURL.py
\$\endgroup\$
  • \$\begingroup\$ so http://www.example.co.uk is not a valid URL? \$\endgroup\$ – Mathias Ettinger Jul 5 at 9:28
  • \$\begingroup\$ It is is valid. \$\endgroup\$ – coder Jul 5 at 9:31
  • 3
    \$\begingroup\$ Oh, because findall and not match, right. But this is confusing from the regex alone… \$\endgroup\$ – Mathias Ettinger Jul 5 at 9:35
3
\$\begingroup\$

Variable Names

Variables should be lower-cased and snake-case, meaning that multiple words should look like some_variable, not someVariable or SomeVariable. I think the name you've chosen for your regex is good and expresses exactly what it is:

protocol_regex = re.compile(...)

Checking length of a list/container data structure

It is not considered pythonic to check the emptiness of a container in python by using if len(container) == 0, most* containers have a __bool__ method built-in that allows you to do if not container for an empty one and if container for non-empty:

from collections import deque

if not {}:
    print("empty dict")
empty dict

if not []:
    print("empty list")
empty list

if not '':
    print("empty string")
empty string

if not deque():
    print("empty deque")
empty deque

if ['result1']:
    print('non-empty list')
non-empty list

One of the few that does not behave this way is queue.Queue, which I've included to give context to the most comment.

With this in mind, change your match check to:

if matches:
    # rest of code
else:
    # else code here
\$\endgroup\$
  • 2
    \$\begingroup\$ Your point on multi-line strings and the regex would usually be correct, but not in this case: the re.VERBOSE flag makes sure that whitespace is ignored and comments are possible, see docs.python.org/3/library/re.html#re.VERBOSE \$\endgroup\$ – RafG Oct 2 at 14:38
  • \$\begingroup\$ @RafG ooo, missed that one. Time for some reading. Good catch, I'll make an edit \$\endgroup\$ – C.Nivs Oct 2 at 14:42
  • \$\begingroup\$ @RafG I suppose that raises a question, is it better practice to use re.VERBOSE and triple-quote your regex, or to use parentheses and break up the raw strings for multiline statements? Asking for my own enrichment here \$\endgroup\$ – C.Nivs Oct 2 at 14:44
  • 1
    \$\begingroup\$ There are some advantages to breaking up the raw strings for sure: syntax highlighting to more easily distinguish comments and no need to escape whitespace characters if the regex contains any. On the other hand,re.VERBOSE feels less 'heavy' to me (admittedly this is very subjective) and is somewhat more flexible. E.g., you can 'indent' parts of your regex to make the structure clearer (example), the string with the regex and comments can be loaded from a file, etc. \$\endgroup\$ – RafG Oct 3 at 8:11
  • 1
    \$\begingroup\$ Loading the regex from a file seems extremely advantageous. Never knew that could be a use case, thanks! \$\endgroup\$ – C.Nivs Oct 3 at 13:46
3
\$\begingroup\$

The regular expression is inadequate for real domain names. Here's a few actual examples that fail:

http://hes.scot/
https://WWW.historicenvironment.scot/
https://www.bbc.co.uk
https://WWW.BBC.CO.UK

These are extracted as

http://hes.sco
https://WWW.his
https://www.bbc.co
https://WWW.BBC

Domain names are case-insensitive, can contain any number of levels, and need not end in a component having 2 or 3 letters.

(Note that the scheme name - http or https - is case-sensitive, as is the local part of a URL).

\$\endgroup\$
0
\$\begingroup\$

There is no need for reinvention of the wheel and complex pattern matching. Use urllib.parse.urlparse to check for the URL's scheme.

from urllib.parse import urlparse

def is_http_or_https(url):
    return urlparse(url).scheme in {'http', 'https'}
\$\endgroup\$
  • 8
    \$\begingroup\$ That might be true, but the OP took this from a book to learn Python coding by example, and I'm pretty sure "use a library" was not the main goal of that chapter. \$\endgroup\$ – AlexV Jul 5 at 10:51
  • 2
    \$\begingroup\$ I am here to review given code to help improve it. If some random coding learning book insists on not using libraries designed to solve a specific problem, the best purpose it can serve is as a combustible. \$\endgroup\$ – Richard Neumann Jul 5 at 13:18
  • 2
    \$\begingroup\$ No need to get destructive here. The author chose to use the URL scheme as an example to learn something about regular expressions. One might argue if that is the best example to teach them, but it's certainly a recognizable one. \$\endgroup\$ – AlexV Jul 5 at 13:28
  • \$\begingroup\$ Yes, you can offer that feedback, however I think it's easy to argue that this suggestion really doesn't improve the code or its intended use. Especially given that it's tagged reinventing-the-wheel, showing that OP specifically intends to circumvent existing libraries to work on a specific skill, which in this case is regex \$\endgroup\$ – C.Nivs Oct 2 at 14:32

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.