Re: [lmi] Code review: product editor

[Evgeniy Tarassov]

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
class?

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
private:
   [...]
};


--
With best wishes,
Evgeniy Tarassov