谢谢homer替我review了代码。看来还是有很多细节地方没有注意到:-(。对于合并constants.py, lib.py, errors.py.我最开始的想法是constants.py<http://errors.py.xn--constants-fv0qg27bevff5dywtq3cvr9abu1b.py> 用来存放程序运行需要的，但是用户不可见的常量。用这些常量是为了之后开发要改的话比较方便。 lib.py主要是存放程序必须的辅助函数 errors.py存放自定义异常类。 把这3个功能不太一致的类放在同一个文件里，会不会分工不清？还是说不须考虑这些呢？
在 2010年7月27日 上午10:36，Homer Xing <[email protected]>写道： > Iaml，首先请原谅我哈，答应你上周做 Peer review 的，但是去改 bug 耽误了 review。 > > 我基于你在 18 日的代码做了 Peer review。并且我的 review 只涉及代码的单行优化。 > > bcos_backup_gtk.py 里: > > pygtk.require("2.0") 删去 > > super(BCoS_Backup_GTK, self).__init__(False, 5) 换成 gtk.HBox.__init__(self, > False, 5) > > left_vbox.pack_start(select_hbox, False, False, 0) 换成 > left_vbox.pack_start(select_hbox, False) > > config().get_instance().get_option_path() 换成 > Config.get_option_path()。Config 类适当改写下: > > class Config: > @classmethod > def get_option_path(cls): > > send_dialog.destroy(); 最后的分号去掉 > > window.set_size_request(400, 400) 换成 window.set_default_size(400, 400) > > backup.py 里: > > sys.path.append("../../") 改成绝对路径吧 > sys.path.append(os.path.abspath("../../")) > > f = open(archive_full_path) > attach = MIMEImage(f.read(), _subtype = subtype) > f.close() > 换成 > with open(archive_full_path) as f: > attach = MIMEImage(f.read(), _subtype = subtype) > > 讨论: constants.py 里的常量都放到 lib.py 里如何？ errors.py 里的类也放到 lib.py 吧 > > constants.py 里 > > BCoS_Default_Metadata_Path = > "/home/iaml/Project/ailurus/ailurus/fedora/BCoS/.bcos_metadata" 换成 > BCoS_Default_Metadata_Path = > os.path.expanduser("~/Project/ailurus/ailurus/fedora/BCoS/.bcos_metadata") 吧 > > lib.py 里 > > class config(): 换成 class Config: > > self.__conf 换成 self._conf。 所有双下划线开头的变量，都换成单下划线开头的。 > > if not os.path.exists(self.__config_path): > # test > if __name__ == "__main__": > print "config does not exist" > self.set_default() > else: > self.__configParser.read(self.__config_path) > > 别这么写。 > > 换成: > > if not os.path.exists(self.__config_path): > if DEBUG: > print "config does not exist" > self.set_default() > else: > self.__configParser.read(self.__config_path) > > > # convert ~ or $ to home directory > for i in range(len(path_list)): > path_list[i] = path_transform(path_list[i]) > if not os.path.exists(path_list[i]): > raise PathNotExists, path_list[i] > 改用 os.path.expand_user 更好哈 > > > -- 感谢抽时间阅读我的邮件 祝生活愉快 林 涛
_______________________________________________ Mailing list: https://launchpad.net/~ailurus Post to : [email protected] Unsubscribe : https://launchpad.net/~ailurus More help : https://help.launchpad.net/ListHelp
