使用 Windows Python 2.3 的 poplib 重构

Question

大家好，你们能帮我把这个代码改得更符合Python的风格吗？

import sys
import poplib
import string
import StringIO, rfc822
import datetime
import logging

def _dump_pop_emails(self):
    self.logger.info("open pop account %s with username: %s" % (self.account[0], self.account[1]))
    self.popinstance = poplib.POP3(self.account[0])
    self.logger.info(self.popinstance.getwelcome()) 
    self.popinstance.user(self.account[1])
    self.popinstance.pass_(self.account[2])
    try:
        (numMsgs, totalSize) = self.popinstance.stat()
        for thisNum in range(1, numMsgs+1):
            (server_msg, body, octets) = self.popinstance.retr(thisNum)
            text = string.join(body, '\n')
            mesg = StringIO.StringIO(text)                               
            msg = rfc822.Message(mesg)
            name, email = msg.getaddr("From")
            emailpath = str(self._emailpath + self._inboxfolder + "\\" + email + "_" + msg.getheader("Subject") + ".eml")
            emailpath = self._replace_whitespace(emailpath)
            file = open(emailpath,"wb")
            file.write(text)
            file.close()
            self.popinstance.dele(thisNum)
    finally:
        self.logger.info(self.popinstance.quit())

def _replace_whitespace(self,name):
    name = str(name)
    return name.replace(" ", "_")   

另外，在这个叫做_replace_whitespace的方法里，我想加一个清理的过程，把所有可能导致处理出错的非法字符去掉。

简单来说，我想把邮件以标准的方式写入收件箱目录。

我这样做是不是有什么问题呢？

Answer 1

关于我对约翰回答的评论

我发现问题出在哪里了，名字字段和主题字段里有一些非法字符，这让Python感到困惑，因为它试图把电子邮件当作一个目录来写，结果看到“:”和“/”后就出错了。

约翰提到的第4点不管用！所以我还是保持原样。另外，第1点是对的吗？我有没有正确地按照你的建议去做？

def _dump_pop_emails(self):
    self.logger.info("open pop account %s with username: %s", self.account[0], self.account[1])
    self.popinstance = poplib.POP3(self.account[0])
    self.logger.info(self.popinstance.getwelcome()) 
    self.popinstance.user(self.account[1])
    self.popinstance.pass_(self.account[2])
    try:
        (numMsgs, totalSize) = self.popinstance.stat()
        for thisNum in range(1, numMsgs+1):
            (server_msg, body, octets) = self.popinstance.retr(thisNum)
            text = '\n'.join(body)
            mesg = StringIO.StringIO(text)                               
            msg = rfc822.Message(mesg)
            name, email = msg.getaddr("From")
            emailpath = str(self._emailpath + self._inboxfolder + "\\" + self._sanitize_string(email + " " + msg.getheader("Subject") + ".eml"))
            emailpath = self._replace_whitespace(emailpath)
            print emailpath
            file = open(emailpath,"wb")
            file.write(text)
            file.close()
            self.popinstance.dele(thisNum)
    finally:
        self.logger.info(self.popinstance.quit())

def _replace_whitespace(self,name):
    name = str(name)
    return name.replace(" ", "_")   

def _sanitize_string(self,name):
    illegal_chars = ":", "/", "\\"
    name = str(name)
    for item in illegal_chars:
        name = name.replace(item, "_")
    return name

Answer 2

我觉得这段代码没有什么明显的问题——它是表现不正常，还是你只是想要一些通用的代码风格建议呢？

几点建议：

1. 用 "foo %s %s", bar, baz 代替 logger.info ("foo %s %s" % (bar, baz))。这样做可以避免在消息不打印时进行字符串格式化的额外开销。
2. 在打开 emailpath 的时候加上 try...finally。
3. 用 '\n'.join (body)，而不是 string.join (body, '\n')。
4. 用 msg.From 代替 msg.getaddr("From")。

Answer 3

这段内容不是在重构代码（我觉得不需要重构），但有一些建议：

你应该使用 email 包，而不是 rfc822。把 rfc822.Message 替换成 email.Message，使用 email.Utils.parseaddr(msg["From"]) 来获取发件人姓名和邮箱地址，使用 msg["Subject"] 来获取邮件主题。

用 os.path.join 来创建路径。这样：

emailpath = str(self._emailpath + self._inboxfolder + "\\" + email + "_" + msg.getheader("Subject") + ".eml")

可以变成：

emailpath = os.path.join(self._emailpath + self._inboxfolder, email + "_" + msg.getheader("Subject") + ".eml")

（如果 self._inboxfolder 以斜杠开头或者 self._emailpath 以斜杠结尾，你也可以把第一个 + 替换成逗号）。

这其实没什么大碍，但你可能不应该把 "file" 用作变量名，因为它会和内置类型冲突（像 pylint 或 pychecker 这样的检查工具会警告你）。

如果你在这个函数外部不使用 self.popinstance（看起来不太可能，因为你在函数内连接和退出），那么就没必要把它作为 self 的属性。直接用 "popinstance" 就可以了。

用 xrange 替代 range。

不要只是导入 StringIO，而是这样做：

try:
    import cStringIO as StringIO
except ImportError:
    import StringIO

如果这是一个可以被多个客户端同时访问的 POP 邮箱，你可能想在 RETR 调用周围加上 try/except，以便在无法获取某条邮件时继续执行。

正如 John 所说，使用 "\n".join 而不是 string.join，使用 try/finally 只在文件打开时关闭它，并单独传递日志参数。

我能想到的一个重构问题是，你其实不需要解析整个邮件，因为你只是想复制原始字节，而你只需要 From 和 Subject 这两个头信息。你可以用 popinstance.top(0) 来获取头信息，从中创建一个空内容的邮件，然后用这个头信息。接着再进行完整的 RETR 来获取字节。如果你的邮件很大（解析它们需要很长时间），这样做才有意义。在做这个优化之前，我建议先测量一下。

对于你的函数来清理名字，这取决于你希望名字有多好看，以及你对邮件和主题是否能让文件名唯一的确定性（这似乎不太可能）。你可以这样做：

emailpath = "".join([c for c in emailpath if c in (string.letters + string.digits + "_ ")])

这样你最终得到的就只有字母数字字符、下划线和空格，这看起来是一个可读的集合。考虑到你的文件系统（在 Windows 上）可能是不区分大小写的，你也可以把它转换成小写（在最后加上 .lower()）。如果你想要更复杂的处理，可以使用 emailpath.translate。