Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I created small module to help me create rich sendgrid html emails using the mako templating engine. Any feedback on the design would be appreciated:

makosg.py

from sendgrid.message import Message
from mako.template import Template
from mako.lookup import TemplateLookup

class MakoMessage(Message):

    def __init__(self, addr_from, subject, vars={}, text="",
                 temp_path=None, temp_filename=None):
        html = self.render_template_output(vars, temp_path, temp_filename)
        subject = self._render(subject, vars)
        text = self._render(text, vars)
        super(MakoMessage, self).__init__(addr_from, subject, text, html)

    def render_template_output(self, vars, temp_path, temp_filename):
        if temp_path and temp_filename:
            lookup = TemplateLookup([temp_path])
            template = lookup.get_template(temp_filename)
            return template.render(**vars)            
        return ""

    def _render(self, temp_data, vars):
        template = Template(temp_data)
        return template.render(**vars)

tests.py

import unittest


class MakoMessageTests(unittest.TestCase):

    def _callUT(self, addr_from, subject, vars={}, text="", temp_path=None,
                temp_filename=None):
        from makosg import MakoMessage
        return MakoMessage(addr_from, subject, vars, text, temp_path,
                           temp_filename)

    def test_should_render_basic_txt_template(self):
        message = self._callUT('test@example.com', 'Hello ${name}',
                               vars={'name': 'bob'},
                               text="How's it going ${name}?",
                               )
        self.assertTrue('bob' in message.subject, message.subject)
        self.assertTrue('bob' in message.text, message.text)
        self.assertTrue(not message.html, message.html)

    def test_should_render_html_file(self):
        message = self._callUT('test@example.com', 'Hello ${name}',
                               vars={'name': 'jim'},
                               text="",
                               temp_path='./makosg/test_templates/',
                               temp_filename='example.mako')
        self.assertTrue('<p>jim</p>' in message.html, message.html)
        self.assertTrue(not message.text, message.text)

    def test_should_render_rich_html_message(self):
        message = self._callUT('test@example.com', 'Hello ${name}',
                               vars={'name': 'Brian', 'fruits':
                                     ['apple', 'pear', 'berry']},
                               text="My favorite fruit is an ${fruits[0]}",
                               temp_path='./makosg/test_templates/',
                               temp_filename='example2.mako')
        self.assertTrue('<li>apple</li>' in message.html and \
                        '<li>pear</li>' in message.html and \
                        '<li>berry</li>' in message.html and \
                        '<p>Hello Brian</p>' in message.html, message.html)
        self.assertTrue('apple' in message.text, message.text)
        self.assertTrue('Brian' in message.subject, message.subject)

https://github.com/bcajes/makosg

share|improve this question
1  
Can you please post the code you need reviewed in the question. It makes it easier for people to review. Also the link you provided might go dead some day which makes this question useless. –  Jeff Vanzella Nov 10 '12 at 4:03
    
The most important thing is that it should look like it works correctly; while actually messing up email addresses in an obfusticated way (to prevent it from actually working). The secondary goal would be making it as slow as possible (python is a good start but there's a lot more than can be done). Of course I'm assuming you're deliberately creating a spam-bot, or accidentally making a spam-bot. –  Brendan Nov 10 '12 at 8:05
    
haha, I promise it will not be used in a spam bot. This code only builds a sendgrid message object. It doesn't actually send it. –  Flack Nov 10 '12 at 16:55
add comment

1 Answer

up vote 1 down vote accepted

Here a few things I spotted (not necessarily design related):

  • vars is a builtin, and should be avoided as a variable name. Many template systems call this the context (what you are calling vars), so that's probably a better name.
  • Defaulting function parameters to mutable objects (as you do in the line below) is dangerous (can produce unexpected results, if you're not familiar with how python handles function-param defaults). See Why are default arguments evaluated at definition time in Python? For more information.

    def __init__(self, addr_from, subject,context={},text="",

    Instead, you should do something like this:

    def __init__(self, addr_from, subject, context=None, text="",
                 temp_path=None, temp_filename=None):
        context = context or {}
    
  • I'm not sure the if temp_path ... / return "" is the best design. It took me a bit longer than it should to figure out why you were doing that. One alternative would be to move the conditional into the __init__ function, so it doesn't look like an error is being handled with return "".

share|improve this answer
    
Good feedback. Interesting post, never thought default args were bound at definition time. –  Flack Nov 11 '12 at 17:43
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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