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

[Daniel Burrows]

  (sorry about dropping the list -- I'm still figuring out the quirks of
this
mail interface)

On Thu, Jul 8, 2010 at 1:05 PM, Piotr Galiszewski <piotr at galiszewski.pl>wrote:

> 2010/7/8 Daniel Burrows <dburrows at google.com>:
> > On Thu, Jul 8, 2010 at 1:39 AM, Piotr Galiszewski <piotr at galiszewski.pl>
> > wrote:Thanks for the informations. I am now rebasing and reworking my
> previous commits. I have planned to firstly write features, and later
> polish, cleanup and refactor code. As I see, this should change now,
> and I should try write code that is working and ready for merging? If
> so, I have to change my working plan a little, cause there is already
> small delay due to this code reworking. Please forgive me this delay
> ;)
>

 Yeah, I know this will introduce some delays, at least for now.  The thing
is, my experience is that deferring code quality often means you never
get around to it.  And getting your code into good shape immediately has
some benefits:

  - It's a lot easier in the long run to have code that's well-formatted,
well
    documented, etc, to start with, rather than going back and spending
    blocks of time doing nothing but documenting and formatting code (IMO).
  - Making sure the design is sensible up-front can save you a *lot* of time
    later; it's relatively cheap to change the design when you're first
putting
    it together, and a lot more expensive to do it after you've built a lot
of
    code.

  Also, my experience with SoC (based on both times that I've been a
mentor) is that it goes better if your code is ready for submission from
early on.  It ensures that we catch any differences of opinion about design,
syntax, etc, early on and not after you have three months worth of code
written.  Plus, this is an educational program, and I think code reviews
serve both to help you write better code, and to give you some experience
with a more formal / professional style of development.

I have two other questions: Do you prefer smaller commits or one
> bigger, full feature commit for each branch? Also API docs should be
> written now, or it could wait a little. Also how precise they should
> be?


  My general preference (especially when I'm reviewing them) is for smaller
commits, balanced by my preference for commits that compile and work.
i.e., try to split your commits up into small chunks that will compile, pass
the unit tests, and run without major bugs (even if not all the features
work yet).  So a good first (or maybe even second or third) stage for your
Qt frontend would be enough code to initialize Qt and display a blank
window.

  But despite what I said, feel free to use your common sense -- e.g., if
it's
going to impose a lot of extra work to split a smallish commit into
two even smaller ones, don't bother.

  As for API docs, please write them up-front.  My experience since I
adopted
this strategy has been that it's a really good way to flush out badly
factored
code and other poor designs (such code is usually hard to document, or you
end up writing documentation that makes you feel so dirty and ashamed
that you go and fix it :) ).  Also, it really helps when other people (ie,
me) are
reviewing your code, so they know what was intended and what's a bug.

  The API documentation should be written as if you're documenting a
library,
because you are (every piece of aptitude is a library from the point of view
of the rest of the program).  The most important thing to document is what
the meaning of each piece is and how it fits into the overall picture.
 Think
about what a reader who encountered your code without knowing what you
were doing would ask.  e.g., consider the Filter class:

   class Filter : public QObject
   {
      Q_OBJECT

         QString _name;

      public:
         Filter(QString name) : _name(name) { }

         const QString &name() { return _name; }

         virtual bool acceptPackage(Package *package) { return true; }

      signals:
         void filterChanged();
   };

  Questions:
     - What is a Filter?
     - How does Filter relate to Package?
     - Under what conditions is filterChanged emitted?

   Here's a stab at answering them, although I'm not sure that I got the
right
answers:

  /** \brief A generic filter on packages for use with FiltersModel.
   *
   *  A Filter represents a predicate on Package objects, implemented
   *  by acceptPackage().  Each filter has a name, which represents
   *  something about the filter.  I'm not sure exactly what.  Maybe
   *  it's intended to appear as a label when the user is selecting
   *  filters?
   *
   *  Filters might change over time; if the logical value of a Filter
   *  changes, it will emit filterChanged().
   *
   *  The main implementation of Filter is SearchFilter, which supports
   *  simple string matches against several fields of a package.
   */
  class Filter : public QObject
  {
    Q_OBJECT

    // \todo Shouldn't this be const?
    QString name;

  public:
    /** \brief Create a new filter with the given name. */
    Filter(const QString &_name) : name(_name)
    {
    }

    /** \brief Retrieve the name of this filter. */
    const QString &get_name() const { return name; }

    /** \brief Test whether the given package matches this filter
     *  and return \b true if it does.
     *
     *  The default implementation of this method matches all packages.
     */
    virtual bool acceptPackage(const Package *package)
    {
      return true;
    }

  signals:
    /** \brief Emitted when this filter is changed to match a different
     *  set of packages.
     */
    void filterChanged();
  };

  I have some more design notes on this particular piece of
code, but I'm waiting to verify that it's OK for me to post some
example code I wrote at work.

 >   So, if I understand correctly, you intend to allocate shared Package
> > objects and pass smart pointers to them?  Would you mind using
> > boost::shared_ptr so we can have consistency across frontends?  (the
> > backend objects that use this pattern already use that wrapper, and I'd
> > rather not add even more types of smart pointers if we can avoid it).
> >
>
> Not exactly, but yes. I will try to use boost::shared_ptr instead of
> it. Should it be done during reintroduction of Package class, or it
> can wait? Probably I will also create package_manager class for
> managing packages. Reading cache in model is not perfect solution and
> probably it will be problematic in the future
>

  Is package_manager some sort of a global storage pool of Packages,
then?  Could you maybe call it package_pool instead to avoid naming
confusion?

  I don't really understand the whole purpose of the Package object.
Is the goal here to save memory and CPU time by only copying package
information into QStrings once per package?


> >>
> >> >    - Why are all of PackageDelegate's methods const when they modify
> >> >      its state?  Something seems wrong here.  Is there a better way
> >> >      to do this?
> >> >
> >>
> >> I have only reimplemented functions from QAbstractItemDelegate. They
> >> all are const there:
> >> http://doc.trolltech.com/4.6/qabstractitemdelegate.html
> >
> >   That would kind of tie your hands.  And all the workarounds are really
> > no better IMO.
>
> It is a normal behavior of this classes, so we have to live with this
> ;) Qt Model Framework is really nice and powerful solution and there
> are no better options for Qt.
>

  Yeah.  Um, my language above was a little ambiguous, so I just wanted to
say clearly that I agree that you have to do things like this given how Qt
works.

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