9
\$\begingroup\$

You are writing your web-page and relentlessly adding links, as you think that links are the best thing a web-page can offer.

The fact is that you write links even to pages that do not exist just yet, because you will write them later.

But later becomes a lot later and now you have your website in your directory with a lot of links pointing to nothing.

You decide to systematically write a page for each link, as to eliminate all the pending links, and using this script to list all such pending links:

An example output looks like:

['addition.html', 'definition.html', 'division.html', 'infinity.html', 'multiplication.html', 'primitive_concept.html', 'recursion.html', 'set.html', 'subtraction.html']

The code is very straightforward, but there is always potential for improvement:

"""
Given a folder of html pages, gives a list of all the pages that are linked to,
but do not exist.
"""
import doctest
import itertools
import os
import re

PATH = os.path.dirname(os.path.realpath(__file__))
flatten = itertools.chain.from_iterable

def destinations(html):
    """
    >>> destinations('''<p>The natural numbers are an <a href="infinity.html">infinite</a><a href="set.html">set</a>defined <a href="recursion.html">recursively</a>as follows:''')
    ['infinity.html', 'set.html', 'recursion.html']
    """
    return re.findall('[a-z_]+\.html', html)

def read(f):
    with open(f) as c:
        return c.read()

def missing_pages(directory=PATH):
    """
    Lists all the pending links of the html pages if the `directory`.
    """
    all_pages = sorted(set((flatten(destinations(read(i)) for i in os.listdir(directory)))))
    return list(i for i in all_pages if i not in os.listdir(directory))

if __name__ == "__main__":
    doctest.testmod()
    print(missing_pages())
\$\endgroup\$
2
  • \$\begingroup\$ You're only reading/scanning the html files right ? What if you have link in a file that is either a JS that generate html or template file or anything dynamic ? You may not cover those yet but I think it could be useful but that would now be framework/technology dependant. \$\endgroup\$
    – Marc-Andre
    Dec 15 '15 at 19:37
  • 1
    \$\begingroup\$ @Marc-Andre this is intended to be used for plain static hand-generated html files \$\endgroup\$
    – Caridorc
    Dec 15 '15 at 19:38
7
\$\begingroup\$

If the script works fine for you the points about robustness below won't matter too much.

  • The way PATH is defined is a bit unusual since in most cases I want to be able to invoke the script from any directory, but this default forces it to the directory of the script. I'd almost say that the normal default argument of os.listdir, namely "." is way better.
  • os.listdir is called too often, the result can just be reused.
  • Don't use list when you can have literal list syntax instead.
  • The output is fine I guess, except for passing it to other scripts you'd usually want a more "standard" format, i.e. a single file per line without quotes.

For the record missing_pages should look more like this (filtering filenames and directories is left as an exercise though):

def missing_pages(directory="."):
    """
    Lists all the pending links of the html pages of the `directory`.
    """
    files = os.listdir(directory)
    all_pages = sorted(set(flatten(destinations(read(i)) for i in files)))
    return [i for i in all_pages if i not in files]

Otherwise looks good, test does what it says, docstrings where needed and the script even has a top-level description.

Robustness

I ran the script on a random directory just to see what happens. With Python 3 I get a UnicodeDecodeError; with Python 2 it errors out on a directory - it wouldn't hurt to skip them. It probably should only read files with the correct ending as well.

Next is that the regular expression to match is quite limited; it also will not really deal with the situation where a file is mentioned, but not linked to. The docstring to destinations implies that the function does more than what it can - so you could just leave out all the HTML fluff and just mention the filenames separated by spaces; or, actually document the corner cases, meaning I'd like to have a docstring saying something like "Finds mentions of HTML files in a string. Only lowercase filenames with underscores are returned." or something similar.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I saved this script inside the website name, so for faster running I was a bit unconventional with the PATH. Just nitpicking, os.listdir is called way more than twice in my code, it's called once per file in the folder. What do you mean when you say: actually document the corner cases:? \$\endgroup\$
    – Caridorc
    Dec 15 '15 at 20:39
  • 1
    \$\begingroup\$ Oh good point with more than twice, so it's even worse :3 With corner cases I meant matching being done with a regex and e.g. not handling whitespace and so on; I've amended the answer. \$\endgroup\$
    – ferada
    Dec 15 '15 at 20:55
  • \$\begingroup\$ I guess the problem was the implicit assumption that I made that every named html file is being linked to, while that's not the case. Really precise docstring you have there, I like it :) \$\endgroup\$
    – Caridorc
    Dec 15 '15 at 21:05
2
\$\begingroup\$

You're only importing itertools to get flatten. Unfortunately you can't use from itertools.chain import from_iterable as flatten, but you could still use from itertools import chain. Then you could also move the flatten definition closer, to make it clear where it comes from. I was confused when I looked around for the function definition of flatten, arranging this clearer mitigates that:

import doctest
import os
import re

from itertools import chain

flatten = chain.from_iterable

Including sample data for destinations is good and explains a lot, but at least put in a line that says the basics of the function.

def destinations(html):
    """Use regex to find and return a list of html destinations

    >>> destinations('''<p>The natural numbers are an <a href="infinity.html">infinite</a><a href="set.html">set</a>defined <a href="recursion.html">recursively</a>as follows:''')
    ['infinity.html', 'set.html', 'recursion.html']
    """
    return re.findall('[a-z_]+\.html', html)

This is highly dependent on your data, but it may be faster to get a set of os.listdir(directory) since you're testing in with it a lot.

I would also prefer missing_pages be spread out. There's little reason to put it all on one line, and it serves to confuse since there are 6 different functions being called on that one line.

def missing_pages(directory=PATH):
    """
    Lists all the pending links of the html pages if the `directory`.
    """
    all_pages = flatten(destinations(read(i)) for i in os.listdir(directory))
    all_pages = sorted(set(all_pages))
    return [i for i in all_pages if i not in os.listdir(directory)]
\$\endgroup\$

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.