Code review comment for lp:~therp-nl/server-env-tools/6.1-fetchmail_attach_from_folder

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

Suggested enhancements:

formatting : would have been nice to get the proper spaces added (after commas, before and after assignment equal signs...)

the fetch_mail method (line 416 of the MP) could be split. I would personnally add one method for the body of the "for folder" loop and another for the body of the for message loop. This gains 8 columns, with allows for less line wrapping and clearer code, within the 80 col convention.

line 497: prefer "key in dictionary" to dictionary.has_key(key).

line 632: you are passing a cmp func to sorted to compare by the first element of a tuple. There are several small issues in there:

1. it is more efficient to use the key named argument (passing itemgetter(0) as key function in this case)
2. but it looks to me that the default sorting of Python does exactly what you want
3. you have a local list : using tuple(sorted(algorithms)) eats up unnecessary memory. Just use algorithms.sort() then return algorithms

in the column definitions (line 634 of the MP and following), using double quotes as delimiter removes the need to escape the single quotes inside the various strings, which makes the code easier to read.

review: Needs Fixing (code review, no test)

« Back to merge proposal