current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite
1

I am writing an email parser and cannot decide how much logic should be included in the __init__ function. I know that a constructor should make the object ready for use, but are there best practices?

I've added several statements to my constructor:

def __init__(self, message):
    assert isinstance(message, basestring)

    parsed_message = email.message_from_string(message)

    if self._is_email(parsed_message):
        self.email_obj = parsed_message  
        email_payload = parsed_message.get_payload()
        self.valid_urls = self._find_valid_urls(email_payload)
        # ... some other stuff like looking for keywords etc.

What do you consider the most pythonic way:

  • Should I create a simple email object which possesses several methods that extract and return keywords, included URLs etc?
  • Do you consider it better to process all this information in the __init__ method and putting all results in object variables?
share|improve this question

3 Answers 3

up vote 3 down vote accepted

Actually, I think you should do something like

@classmethod
def parse(cls, message):    
    parsed_message = email.message_from_string(message)

    cls._verify_email(parsed_message) # throws if something is wrong

    message = cls() # make an e-mail message object 
    email_payload = parsed_message.get_payload()
    message.valid_urls = self._find_valid_urls(email_payload)
    # ... some other stuff like looking for keywords etc.

    return message

Which you would then use like

 my_message = Message.parse(message_text)

I prefer to leave the constructor for just basic object construction. It shouldn't parse things or do complex logic. It gives you flexibility because it is now possible to construct an e-mail object that isn't just a parsed version of the message text. I often find that useful for testing, and it reduces the dependance on a particular source for e-mails.

share|improve this answer
up vote 1 down vote

I think you should keep in mind these two principles in mind:

  1. DRY (Don't Repeat Yourself)
  2. SRP (Single Responsibility Principle)

According to SRP, your init function should be just initializing your object. All other stuff should go into their respective functions and called as needed.

email = Email(message)

if not email.is_valid: # Do some stuff

urls = email._valid_urls # use property here
share|improve this answer
up vote 4 down vote

Notes:

  • Personally I prefer to keep the initialization method simple and lean. Just setup the minimum state of the object, let subsequent calls to methods do the real job when it's needed. Otherwise, why use a class? Maybe you don't need a class at all and just a function would do?

  • assert isinstance(message, basestring): Python is a dynamic language, embrace the fact and don't lose a second checking whether every argument has the type you expect (instead, write good docstrigs so the user knows how to call it). Another question is checking that given values satisfy some precondition (i.e assert len(s) >= 10).

share|improve this answer

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.