[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [lmi] Code review: product editor

From: Evgeniy Tarassov
Subject: Re: [lmi] Code review: product editor
Date: Thu, 29 Mar 2007 16:51:11 +0200

On 3/28/07, Greg Chicares <address@hidden> wrote:
On 2007-3-26 13:48 UTC, Evgeniy Tarassov wrote:

However, shouldn't we rewrite this class completely instead?

The fields not included are of course accessed by indexing a
db_names object from the vector returned by this function:
  std::vector<db_names> const& LMI_SO GetDBNames();
Even db_names::Idx is accessed that way, and I think our
discussions have raised the issue of whether that member's value
should be asserted to equal the value of the loop counter in
DatabaseView::SetupControls(). But why index by a sequential
loop counter instead of iterating across an available vector?
The former could break if enumerators ever fail to follow the
pattern 0, 1, 2, ... N-1; but iterating across a vector works
robustly without relying on any such assumption. And why have a
copy of 'LongName' here but not 'ShortName'?

I see two other designs to consider:

(1) This class holds only a DatabaseNames enum. That's enough to
find the corresponding struct db_names in the vector returned by
  std::vector<db_names> const& LMI_SO GetDBNames();
and we can then access that struct's members directly. And we
don't have to know that description() here is 'LongName' there.
Then the only thing we have to worry about is mapping between
that enum and wxTreeItemData::GetId().

This was the original idea, but then it has transformed itself into
caching the data needed and the class became what it is now.

(2) This class holds a db_names struct. To make that work, I guess
we'd have to rewrite that struct (I mean, char* members? in 2007?
Is that required for static initialization to work?) so that its
implicitly-defined member functions do the right thing, and hold a
copy of it here as the itemdata. (I don't know whether that'd make
populating the tree too slow.)

Or maybe even a third:

(3) Mixin programming:

  class database_tree_item_data
      :public wxTreeItemData
      ,public db_names
  {}; // Is any implementation actually required?

There is a feature that should be implemented -- it was mentioned in
Wendy's product editor feedback:
[rephrased] It is desirable to keep axis selection for a particular
product while selecting other products. The simplest way to keep that
axis selection information is to store it in database_tree_item_data
along with db_names.

Since we need to add additional data, it would be better IMHO to use
your (2) version. AFAIU (2) and (3) are the same thing except that in
(3) we dont have to use .member_name to access the db_names data, or
is there any difference between holding data as member or as a base

Something like this (your (2)-nd version + additional data for
preserving axes selection)?

struct database_tree_item_data
 :public wxTreeItemData
   db_names const& db_name() const {return *this;}
   axes_selection& axes_selection() {return axes_selection_;} // +
const version

With best wishes,
Evgeniy Tarassov

reply via email to

[Prev in Thread] Current Thread [Next in Thread]