Roundup Tracker - Issues

Message4212

Author schlatterbeck
Recipients jerrykan, schlatterbeck
Date 2010-12-21.21:53:04
Message-id <1292968385.54.0.740430311126.issue2550576@psf.upfronthosting.co.za>
In-reply-to
Hello John,
I really like the idea of splitting up the mail parsing.
I've taken a closer look at the patch.
I like the way you semantically dissected the large method into smaller
ones.
There are some (small) errors so I've not (yet) accepted it as-is.

The following corrections would be necessary:
- _handle_message_get_pgp_message currently defines the function
  pgp_roles after using it some lines above
  there currently is no regression test for the pgp feature... I'm not
  using it myself.
- the db.commit that commits a user is called two times, once in the
  method building the author _handle_message_get_author_id and once in
  the top-level _handle_message routine. The call in the
  _handle_message_get_author_id method is wrong, it commits before some
  other checks are done.
- some values are recomputed several times. This may introduce
  hard-to-find bugs in the future (and *decreases* maintainability)
  because a change might fix one computation and miss another. 
  One example is the recomputation of message.extract_content in two
  separate methods (and only using one of the two returned values)

When refactoring this I'd introduce a new class, lets call it
"parsedMessage". In this I'd put all the _handle_* methods as class
methods of the new class (stripping the prefix _handle_message). Then
the constructor of the new class would call all your _handle methods and
build the data structures. Finally a method committing all this to the
database would be called by the original _handle_message routine.
Maybe the intermediate commit would also be taken out of the __init__.
This would also avoid lots of passed arguments: Each of the new methods
will introduce some object variables as self.<variable>, e.g., we would
compute the class only once as self.cl (bad example, this *is* later
explicitly recomputed when the database is reopened for the new user)
Maybe we can even add this to the currently existing Message class.

If you have time I'd appreciate if you again work on that patch.
Alternatively I might look into this more closely but this might take
more time.... I'd probably take the route of committing the fixed patch
(attached) and working from there to factor the code into a separate class.

I'm attaching a modified patch that fixes the first two issues above.

Thanks for this work
Ralf
History
Date User Action Args
2010-12-21 21:53:05schlatterbecksetmessageid: <1292968385.54.0.740430311126.issue2550576@psf.upfronthosting.co.za>
2010-12-21 21:53:05schlatterbecksetrecipients: + schlatterbeck, jerrykan
2010-12-21 21:53:05schlatterbecklinkissue2550576 messages
2010-12-21 21:53:05schlatterbeckcreate