[Aptitude-devel] aptitude-qt VCS usage and code review

[Daniel Burrows]

  BTW, regarding the filter class, I have some more comments.  I still
don't know if I can post the sample code I wrote at work yesterday, so
I'm just going to include my comments and leave the code out.



  Looking at this class reminds me of something else: I generally prefer to
avoid concrete virtual classes in favor of interface-based inheritance.
 There
are a bunch of benefits to this; e.g., it usually gives you a conceptually
cleaner
design, it makes it much easier to cut dependencies between components (for
testing, for instance), and it makes it a lot clearer exactly how two
components
interact (instead of the slippery business of self-calls).  This is
different from
how traditional widget sets (like Qt) tend to work, but I think it's worth
the
trouble to get the hang of this style of coding.

  In this case, it looks to me like you have three things that are mixed
together:
    1. The filter function.
    2. The name associated with the filter (always set when the filter is
constructed
        and never changed).
    3. A holder that allows the filter function to be modified to another
filter that's
        implemented by the same class and that emits a signal when it's
changed.

  Item (1) could be accomplished simply by having a member variable of type
boost::function<bool(Package *)>, or by using your own interface.  Probably
a
custom interface is a bit better?  I would avoid Qt dependencies in the
filter
interface (leaving aside Package), although obviously implementations could
rely on Qt objects.  Filters should probably be immutable.

  Item (2) confuses me -- what is the name for, exactly?  If it's just the
name the user gave, why is it immutable?  I'm going to assume that
was an oversight.

  Item (3) can be handled at the same time as item (2), by creating
a separate "holder" class that represents the user's definition of the
filter and is modified when the definition is modified.  This has the
extra advantage of letting you switch to any other filter, not just one
implemented by the same concrete class.

  i.e., you would have "package_filter", which is an interface for
predicates
on packages, and "package_filter_definition", which contains a name and a
package_filter (pointer), both of which have mutators, and which has two
signals corresponding to those fields, emitted when they change.

  Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/aptitude-devel/attachments/20100709/0b0f58c9/attachment.htm>