3
\$\begingroup\$

I've recently re-written a Python script I use to run a couple of lightweight blogs. Looking over the horrible code I'd written before, I decided to rewrite it using object-oriented concepts. I wanted to submit it to get feedback, best practices, and other areas I should look at. This is my first script that uses any object oriented ideas.

import markdown
import jinja2
import re
import datetime
import time
import glob
import cgi

class Article:
    def __init__(self, local_dir, local_file):
        local_file = local_file.replace('/','')
        self.local_file_name = local_file
        with open(local_dir + '/' + local_file) as f:
            self.lines = f.readlines()
        self.file_text = ''.join(self.lines[4:])        

    def text(self):
        return self.file_text

    def title(self):
        title = self.get_metadata(self.lines[0])
        return title

    def html_filename(self):
        html_filename = re.sub('.txt','.html',self.local_file_name)
        return html_filename

    def date_text(self):
        date_text = self.get_metadata(self.lines[2])
        return date_text

    def date_datetime(self):
        date_txt = self.date_text()
        date_obj = datetime.datetime.strptime(date_txt, '%d %B %Y')
        return date_obj

    def date_rss(self):
        date = time.strptime(self.date_text(), '%d %B %Y')
        rss_date = time.strftime('%a, %d %b %Y 06:%M:%S +0000', date)
        return rss_date

    def summary(self):
        summary = re.sub('<[^<]+?>','', self.html())[0:200]
        summary = re.sub('\n',' ',summary)
        return summary

    def html(self):
        md = markdown.Markdown()
        converted_text = md.convert(self.file_text).encode('utf-8')
        return converted_text

    def get_metadata(self,line):
        element = re.sub('\n|Author: |Date: |Title: ','',line)
        element = cgi.escape(element).strip()
        return element

class FileList:
    def __init__(self, dir, ignore_list):
        self.textfiles = glob.glob(dir+"/*.txt")
        for ignored_file in ignore_list:
            self.textfiles.remove(dir+ignored_file)

    def files(self):
        return self.textfiles

class Site:
    def load_articles(self, dir, ignore_list):
        file_list = FileList(dir, ignore_list)
        articles = []
        for file in file_list.files():
            article = Article(dir, file.replace(dir,''))
            articles.append({
                    'title': article.title(),
                    'datetime': article.date_datetime(),
                    'text': article.text(),
                    'summary': article.summary(),
                    'html': article.html(), 
                    'date_text': article.date_text(),
                    'html_filename': article.html_filename(),
                    'date_rss': article.date_rss()
                    },)
        articles = sorted(articles, key=lambda k: k['datetime'], reverse=True)
        return articles

    def build_from_template(self, data, template, output_file, dir):
        with open(template) as f:
            template = jinja2.Template(f.read())
        with open(dir + '/' + output_file,'w') as i:
            i.write(template.render(data = data))
        return True

    def build_site(self, params):
        dir = params['DIR']
        template_dir = params['TEMPLATE_DIR']
        index_template = template_dir + '/index_template.html'
        archive_template = template_dir + '/archive_template.html'
        rss_template = template_dir + '/rss_template.xml'
        article_template = template_dir + '/article_template.html'
        index_output = '/index.html'
        archive_output = '/archive.html'
        rss_output = '/index.xml'       
        site = Site()
        articles = site.load_articles(dir, params['IGNORE_LIST'])       
        for article in articles:
            output = article['html_filename']
            site.build_from_template(article, article_template, output, dir)
        site.build_from_template(articles, index_template, index_output, dir)
        site.build_from_template(articles, archive_template, archive_output, dir)
        site.build_from_template(articles, rss_template, rss_output, dir)   
        return True

Here's the script that executes the code above to actually build a site:

#!/usr/local/bin/python

import pueblo

PARAMS = {
    'DIR': '/dir/to/your/html/files/', # no final slash
    'TEMPLATE_DIR': '/dir/to/store/your/templates', # no final slash
    'IGNORE_LIST': ['ignorethis.txt']
    }

print "Content-type: text/html\n\n"

site = pueblo.Site()
site.build_site(PARAMS)

print "<html><head><title>Site Rebuilt</title></head><body><h1>Site Rebuilt</h1></body></html>"

Finally, here's an example Jinja2 template:

<!DOCTYPE html>
<title>yoursite.com</title>
<link rel="stylesheet" type="text/css" href="style.css">
<meta name="viewport" content="user-scalable=yes, width=device-width" />
<link href="./index.xml" rel="alternate" type="application/rss+xml" />
<h1>yoursite.com</h1>
<p class="site_description">your description</p>
<ul class="navbar">
<li class="navitem"><a href="your_about_page.html">about</a></li>
<li class="navitem"><a href="./archive.html">archive</a></li>
</ul></div>
<div class="article_list">
{% for i in range(30) %}
<h2><a href="{{ data[i].html_filename }}">{{ data[i].title }}</a></h2>
<p>{{ data[i].date_text }}: {{ data[i].summary }} <a href="{{ data[i].html_filename }}">...</a></p>
{% endfor %}
<h2><a href="archive.html">View All Articles</a></h2>
\$\endgroup\$

3 Answers 3

4
\$\begingroup\$
import markdown
import jinja2
import re
import datetime
import time
import glob
import cgi

class Article:
    def __init__(self, local_dir, local_file):

It would make more sense to pass in the complete path, and then use os.path.dirname and os.path.basename to extract the parts if you need them.

        local_file = local_file.replace('/','')

Don't use replace to eliminate items in your string if you know where they are. Use .lstrip() or .rstrip() to remove from the beginning or end. Furthermore, don't use string manipulation on filepaths at all. Use os.path functions

        self.local_file_name = local_file

Don't store things in local variables and then store them on the object. Just store them directly on the object.

        with open(local_dir + '/' + local_file) as f:
            self.lines = f.readlines()
        self.file_text = ''.join(self.lines[4:])        

    def text(self):
        return self.file_text

Rather then define functions like this, just store it as .text and fetch it directly.

    def title(self):
        title = self.get_metadata(self.lines[0])
        return title

    def html_filename(self):
        html_filename = re.sub('.txt','.html',self.local_file_name)
        return html_filename

Don't use regular expressions when a simple string function will do the same. In this case . means something special and so this probably doesn't work exactly like you think.

    def date_text(self):
        date_text = self.get_metadata(self.lines[2])
        return date_text

    def date_datetime(self):
        date_txt = self.date_text()
        date_obj = datetime.datetime.strptime(date_txt, '%d %B %Y')
        return date_obj

    def date_rss(self):
        date = time.strptime(self.date_text(), '%d %B %Y')
        rss_date = time.strftime('%a, %d %b %Y 06:%M:%S +0000', date)
        return rss_date

Generally, it doesn't make sense to have an object provide its data in multiple formats like this. Just provide access to the datetime object. Your caller can use the strftime/strptime methods to get the format they want. It doesn't make any sense for this object to be concerned with the formats other objects want.

    def summary(self):
        summary = re.sub('<[^<]+?>','', self.html())[0:200]

We use +? instead of *?

        summary = re.sub('\n',' ',summary)
        return summary



    def html(self):
        md = markdown.Markdown()
        converted_text = md.convert(self.file_text).encode('utf-8')
        return converted_text

I'd suggest for all these, that you extract the data in the constructor, and store it on local attributes.

    def get_metadata(self,line):
        element = re.sub('\n|Author: |Date: |Title: ','',line)

This ignores the actual information saying what the line is. That bothers me. I think it'd better to follow those pieces of information.

        element = cgi.escape(element).strip()
        return element

class FileList:
    def __init__(self, dir, ignore_list):
        self.textfiles = glob.glob(dir+"/*.txt")
        for ignored_file in ignore_list:
            self.textfiles.remove(dir+ignored_file)

    def files(self):
        return self.textfiles

Again, no need in python to define getters, just access the attributes. The whole thing actually would be better as a function return the list of files.

class Site:
    def load_articles(self, dir, ignore_list):
        file_list = FileList(dir, ignore_list)
        articles = []
        for file in file_list.files():
            article = Article(dir, file.replace(dir,''))
            articles.append({
                    'title': article.title(),
                    'datetime': article.date_datetime(),
                    'text': article.text(),
                    'summary': article.summary(),
                    'html': article.html(), 
                    'date_text': article.date_text(),
                    'html_filename': article.html_filename(),
                    'date_rss': article.date_rss()
                    },)

Don't copy your article over into a dictionary. That completely defeats the point of having object. Make a list of articles and have Jinja extract the fields.

        articles = sorted(articles, key=lambda k: k['datetime'], reverse=True)

Why not do an inplace sort?

        return articles

    def build_from_template(self, data, template, output_file, dir):
        with open(template) as f:
            template = jinja2.Template(f.read())
        with open(dir + '/' + output_file,'w') as i:
            i.write(template.render(data = data))
        return True

    def build_site(self, params):
        dir = params['DIR']
        template_dir = params['TEMPLATE_DIR']
        index_template = template_dir + '/index_template.html'
        archive_template = template_dir + '/archive_template.html'
        rss_template = template_dir + '/rss_template.xml'
        article_template = template_dir + '/article_template.html'
        index_output = '/index.html'
        archive_output = '/archive.html'
        rss_output = '/index.xml'       
        site = Site()

You already have a site object, why are you creating another one?

        articles = site.load_articles(dir, params['IGNORE_LIST'])       
        for article in articles:
            output = article['html_filename']
            site.build_from_template(article, article_template, output, dir)
        site.build_from_template(articles, index_template, index_output, dir)
        site.build_from_template(articles, archive_template, archive_output, dir)
        site.build_from_template(articles, rss_template, rss_output, dir)   

You repeat the same thing for the index/archive/rss without any substational code differences. Have a list of ["archive","rss",index"] and do everything related to those in a loop.

        return True

Serves no purpuse. You don't need a return value unless you are answering a question.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much for this, Winston. Can you describe the in-place sort? I'm not exactly sure how that would work when I don't have a list of articles somewhere ordered by the datetime. \$\endgroup\$
    – Mike Shea
    Mar 23, 2013 at 15:32
  • \$\begingroup\$ @MikeShea, use the .sort() method instead of the sorted() function. Its somewhat more efficient if you don't need to keep the original list. \$\endgroup\$ Mar 23, 2013 at 15:45
  • \$\begingroup\$ Thanks! I just updated it with articles.sort(key=lambda k: k.datetime, reverse=True) instead of articles = sorted(articles, blah). A big help! \$\endgroup\$
    – Mike Shea
    Mar 23, 2013 at 16:59
4
\$\begingroup\$

Overall your design looks good to me. A couple of relatively minor things to note:

Use new-style classes
In Python 2.x you should make sure your classes inherit from object. Such classes are known as new-style classes. (In Python 3.x all classes are new-style classes.) New-style classes offer:

Use the standard-library to make your code more portable
When building up paths you may do so in a cross-platform way by using os.path.join. This is just one example, but there are other places in your code where you can use this:

import os
file_path = os.path.join( local_dir, local_file )
with open(file_path) as f:
    ...

Miscellaneous
There is a comment which doesn't agree with the code.

'DIR': '/dir/to/your/html/files/', # no final slash
\$\endgroup\$
2
  • \$\begingroup\$ Is this a simple matter of doing something like "class article(object):"? \$\endgroup\$
    – Mike Shea
    Mar 25, 2013 at 14:00
  • \$\begingroup\$ Yes exactly: class Article(object): \$\endgroup\$ Mar 25, 2013 at 15:29
1
\$\begingroup\$

There's nothing wrong with directly accessing class attributes in Python, so you could cut out some complexity by doing all your metadata-related calculation once in __init__:

class Article:
    def __init__(self, local_dir, local_file):
        ...
        # I'd actually lose get_metadata() completely, and do its list parsing here as well.
        self.title = self.get_metadata(self.lines[0])
        self.html_filename = re.sub('.txt','.html',self.local_file_name)           
        # etc.

If the attribute is something that's likely to need some manipulation each time you need it, you can use properties as well.

Also, as a general note, you don't really need to assign your return values to a variable before returning them. You can easily do

def title(self):
    return self.get_metadata(self.lines[0]) 

instead of

def title(self):
    title = self.get_metadata(self.lines[0])
    return title
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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