[lmi] Code review: product editor

[Greg Chicares]

I haven't been able to study all this code yet, but want to share
some thoughts sooner rather than later.

(1) Class template MultiDimTableTypeTraits duplicates lmi's
function template value_cast(). I would much rather use the lmi
facility instead. These two facilities differ in their behavior,
for instance in the precision they display. 'NAARDiscount', for
example, needs all available precision; a typical value would be
0.9975397676709933, and 0.997540 is not close enough when we're
dealing with billions of dollars. There may be other, subtler
differences in their behavior. Furthermore, function template
value_cast() is carefully unit tested; and it's easier anyway to
maintain a system with only one facility to meet each need in a
uniform fashion.

(2) I'd prefer to use standard types instead of wx types where
possible. IIRC, wx container types are just synonyms for STL
containers anyway, but the string types are not substitutable.
Sometimes we have no choice, e.g., when deriving from wx classes:
    // wxDocument overrides.
    virtual bool OnCreate(wxString const& filename, long int flags);
but such cases are relatively rare.

I understand the historical reasons for a framework-specific
string class, but in this millennium I feel it's better for lmi
code to use a standard facility instead, even if class wxString
has extensions that might be useful. I looked through the
extensions that are used in the product editor, and these
examples seem representative:

    return wxString::Format("%f", boost::any_cast<double>(value));
    if(!value.ToDouble(&res))
    MDTABLE_TTRAITS_INTEGRAL(unsigned int, ToULong, unsigned long, 0);
    wxString str; str << (col == tgc_limit ? value.first : value.second);
    if(!str.ToDouble(&as_double))

I suspect that lmi's function template value_cast() may meet all
those needs.

(3) Let's try to eliminate macros and especially the boost
preprocessor library from the product editor if possible. Macros
can be useful if there's no reasonable alternative, but they're
error-prone and hard to maintain, so I'd like to make sure we've
considered every alternative first. And, even though we already
use boost, every time we add a dependency on one more boost
library we make the system more difficult to comprehend and to
maintain; again, we should consider every alternative first.

I'd be less hesitant about using an additional library such as
boost::random that merely changes the way we might write one line
of code, and more hesitant about a library such as this one that
fundamentally changes the way we write large pieces of code. Even
so, I thought it a good idea (Vadim's 2006-06-23T12:47Z email) to
replace my hand-coded parser with boost::spirit--we need to judge
each case on its merits. I may be missing something, but here I
don't see a compelling need for the boost preprocessor library.

For example, DatabaseTableAdapter::ConvertValue() seems somewhat
challenging to me. After reading the documentation for it:
    /// Helper, converts array of boost::any into array of ints
    void DatabaseTableAdapter::ConvertValue
        (Coords const& coords
        ,std::vector<int>& indexes
        ) const
and for its 'Coords' argument type:
    /// Coordinates for an element of the table
    typedef std::vector<boost::any> Coords;
I wonder whether it's really necessary at all. If coordinates are
only table indexes, then why can't we just use a vector of 'int'
directly?

And class MultiDimTable##n seems to be used only with 'n' values
of one and seven. Suppose we just wrote two distinct class
templates that didn't use the preprocessor--or especially the
boost preprocessor library, which I think we could then eliminate
entirely. Then, I bet we could find some other means of making it
generic wrt 'n': a non-type template parameter, perhaps. Even if
we couldn't, I might feel more comfortable about maintaining two
separate, non-preprocessed class templates, especially because I
don't think we'll need more than two.

(4) I dread dereferencing a null pointer. That can cause a crash.
One important difference between lmi and many similar systems is
that ours crashes less often. That makes a tremendous difference
to end users. And dereferencing a dangling, uninitialized, or
null pointer can cause mysterious, intermittent problems that are
difficult to diagnose.

One way to make sure we never dereference a null pointer is not
to create a pointer in the first place. I think that's always the
best way when it's possible. Thus, here:

    stratified_entity* TierDocument::get_stratified_entity(e_stratified index)
    {
        return &charges_.raw_entity(index);
    }

a reference seems preferable because the pointer cannot be void.
I worry especially about code like this:

    std::vector<double>* limits_;
    std::vector<double>* values_;

It's accompanied by numerous null-pointer checks, but it's hard
to be sure those checks are comprehensive, or that they'll remain
comprehensive after future changes. Using reference members would
prevent that (as long as no assignment operator is necessary); so
would copies (as long as reference semantics are not required).

Alternatively, should we redesign class tier_entity_adapter
(where this usage occurs), or class stratified_entity, or both?
The adaptor's rationale is:

    /// We can't store a pointer/reference to stratified_entity because
    /// stratified_entity has private interface for accessing
    /// its limits_ and values_ data members. Therefore we are storing
    /// references directly to its inner data members.

Class stratified_charges gains access to those members through
friendship; would friendship suffice for the tier_entity_adapter
class as well--or should the existing accessors just be made
public? Alternatively, the adaptor seems to add only four
functions:

    double_pair get_value(unsigned int band) const;
    void set_value(unsigned int band, double_pair const& value);
    void set_bands_count(unsigned int n);
    unsigned int get_bands_count() const;

so would it be better just to add them to class stratified_entity
and eliminate the adaptor? Or, better still: with public const
accessors added to class stratified_entity, could these four be
written as non-friend non-member functions, passing and returning
stratified_entity objects by value? I'm guessing that would be
enough because class TierDocument holds a stratified_charges
object, which holds a std::map of stratified_entity objects...by
value, anyway, so we can just replace map elements instead of
modifying them through writable references.

(5) I've come to believe that 'new' and especially 'delete' are
generally to be avoided, with only a few exceptions where objects
can't be created on the stack instead of on the heap:

 - in low-level library code that's separate from the application
   layer and is carefully tested, such as 'any_member.hpp'

 - when it's necessary to delete resources provided by a library
   such as wx, in order to avoid leaks; or to furnish resources
   to such a library that insists on owning them or requires that
   they be created on the heap

 - when RAII is truly appropriate for other reasons, in which
   case we wouldn't invoke 'delete' ourselves:

  http://www.boost.org/libs/smart_ptr/shared_ptr.htm#BestPractices
| Every occurence of the new keyword in the code should have the form
| shared_ptr<T> p(new Y); [or some other smart pointer]
| If you observe this guideline, it naturally follows that you will
| have no explicit deletes

Thus, for example, I question code like this:

    MultiDimAxis<enum_gender>* DatabaseTableAdapter::GetAxis0()
    {
        return new DatabaseGenderAxis();
    }

Even if the worst thing that can happen is a leak, leaks can be a
problem when an application is kept open for a long time: that's
certainly a major issue with firefox. I'm not sure I see how this
function is used (apparently through some macros), but couldn't
such an object be maintained on the stack instead?

I've arrived at opinions like this because of mistakes I've made.
I wince when I look at code I wrote many years ago that violates
these guidelines. Sometimes there was a historical justification:

  // TODO ?? Apparently the original reason for using smart pointers
  // was to minimize stack usage in a 16-bit environment; clearly that
  // doesn't matter anymore.

but more often there was none. The smart-pointer RAII technique
is not always appropriate: the above comment applies to code like

    boost::scoped_ptr<OLCommFns> CF(new OLCommFns(q, i));

that would much better be written as

    OLCommFns CF(q, i);

And "wince" isn't strong enough for the special member functions
of class TDBValue, which the compiler would generate correctly if
std::vector were used instead of arrays of int on the heap.