[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: Wed, 21 Mar 2007 18:36:14 +0100

On 3/21/07, Greg Chicares <address@hidden> wrote:
On 2007-3-21 4:31 UTC, Greg Chicares wrote:
> Now I'd like to pause to discuss the next two, but let me
> do that in a separate email.

(I suppose these comments would also apply to

Pointers fill me with trepidation.

RCS file: /sources/lmi/lmi/database_view.cpp,v
       DatabaseTreeItemData* item_data = dynamic_cast<DatabaseTreeItemData*>
  -    if(!item_data)
  -        {return;}

The nice thing about this early return was that I could tell
immediately that this pointer wouldn't be dereferenced later if
it was NULL here. But maybe the early return was part of the
problem that needs to be solved.

I removed it to deal with a potential "bug" in wx. I'm not sure that
wxTreeCtrl guarantees that it will never select a root node when it is
hidden. So the previous version of product editor simply bailed out as
soon as it detected that we had to deal with the root node.

The patched version deals with it, because even if it happens somehow
we could simply recover from it just by showing an empty grid.

Still, I wonder why we maintain a pointer here. I understand that
wxTreeCtrl::GetItemData() returns a pointer. I understand that
it's a wxTreeItemData*, and that we need a dynamic_cast to make
sure it's a DatabaseTreeItemData*. Presumably we'd maintain a
pointer variable (instead of immediately dereferencing it and
using a DatabaseTreeItemData&) only if it could be NULL. What I
wonder is how this could be NULL.

I see two different ways. First,
is non-NULL, but
is not. I guess that would be a logic error: we own the treectrl,
and we wouldn't put itemdata of any other type into it. Let's
keep this question in the back of our minds: "should we trap
that potential logic error?".

Second, it could happen that
is NULL. But I guess that's under our complete control, in
DatabaseView::SetupControls() (I'm assuming that any tree node
can have itemdata). Examining that function, it looks like a node
has NULL itemdata iff this condition is met:
  if(name.Idx == name.ParentIdx)
and apparently that's true only for the root. Suppose we give
the tree root a non-NULL object for its itemdata; then could we
give SetupControls() a postcondition like "Every node has non-
NULL itemdata"?

If we can do that, then there's only one way for 'item_data'
to be NULL later, and that's a logic error as discussed above,
so we could just assert that it's not NULL. Untested idea:

  wxTreeItemData* p = tree.GetItemData(event.GetItem());
  DatabaseTreeItemData& item_data = dynamic_cast<DatabaseTreeItemData&>(*p);

The dynamic_cast (if I got it right) then throws a standard
exception in the logic-error case, and existing code should catch
that and handle it. That seems tidier than

  S* p = whatever();
  T* temporary = dynamic_cast<T*>(p);
  LMI_ASSERT(NULL != temporary);
  T& variable_we_really_use = *temporary;

and I think it's a good idiom because dynamic_cast<T&> is
defined that way by C++2003 5.2.7/6-9 . (Tell me if your opinion
differs, though).

An explicit assert is slightly better from the point of view of
verbosity of the error message -- if we rely on std::runtime_error
exception generated by dynamic_cast then we loose the filename and the
line number where the exception was generated, which could be useful
for error reporting and debugging. Imagine a error popping out to a
user. If it is an exception caught from dynamic_cast, then user does
not have the precious information (filename and line number) to
transmit, so that it could be quickly found and fixed.

BTW, maybe it should be const:
  DatabaseTreeItemData const& item_data = ...

Yes, i should have made it const. Sorry.

Now, why do I belabor this pointer stuff? Let's scan ahead:
  +    bool const is_leaf = item_data && 
Okay, 'item_data &&' guards against NULL. (Is that needed?)
  +    if(item_data)
  +        {
  +        item_label = item_data->GetDescription();
  +        }
Okay, 'if(item_data)' guards against NULL.
  +    if(is_leaf)
  +        {
  +        selected_tdbvalue = document().GetTDBValue(item_data->GetId());
  +        }
I got stuck there: does 'if(is_leaf)' guard against 'item_data'
being NULL? I suppose it does, somehow, else you'd have written
it otherwise. Having studied this much now, I think I can even
explain why it does (though I couldn't see it at a glance): it's
because of an implicit postcondition of SetupControls(). That
postcondition isn't
  "Every node has non-NULL itemdata of type DatabaseTreeItemData*"
though: it's
  "Every node has non-NULL itemdata of type DatabaseTreeItemData*,
  with the sole exception that the root node has NULL itemdata
  that you mustn't dereference."
But some later change might break that postcondition and cause a

Oh, actually it is simply because whenever item_data is NULL, is_leaf
is false, because:
+    bool const is_leaf = item_data && !tree.GetChildrenCount(event.GetItem());
It does not rely on any postcondition from SetupControls. But now i
see that i should have written it another -- more clear way.

Other comments:

  -    std::size_t index = item_data->GetId();

I'm glad to get rid of this. At first, I thought of
  int wxEvent::GetId() const;
and worried about an implicit conversion from signed to unsigned.
Now I look it up and see it's wxTreeItemData::GetId(), which
returns a wxTreeItemId by const reference, but still I wouldn't
know whether that's losslessly convertible to std::size_t. By
passing it directly

Ugh, I'm sorry, that is my fault: item_data is of type
'DatabaseTreeItemData*' which derives from wxTreeItemData. And by
coincidence i have named acessor for our own custom field
DatabaseTreeItemData::id exactly as one of non-virtual methods of
wxTreeItemData (wxTreeItemData::GetId).

In the code item_data->GetId() refers to
std::size_t DatabaseTreeItemData::GetId() const;

Now looking at it -- the class DatabaseTreeItemData should be moved
from databae_view_editor.hpp to database_view.cpp, and it methods
should follow lmi naming convention.

BTW I wanted to ask you before if you want me to rename product_editor
related classes from SomeName to some_name. I should have done that
before :S, but better late than never..

I think we could simplify this by making sure every node has
non-NULL itemdata. Then the original
  -    SetLabel(item_data->GetDescription());
should be reliable.

Yes, that should be the best way to clear the code.

  +    TDBValue* selected_tdbvalue = NULL;

Let's hope we don't ever dereference this....

In DatabaseTableAdapter every time the inner tdbvalue is accessed it
is checked to be non-NULL. I have tried to enforce it by requiring a
non-NULL pointer but it makes the code less understandable since  a
dummy static TDBValue has to be used and additional checks should be
added so that this dummy variable is not modified by mistake.

  +    if(is_leaf)
  +        {
  +        selected_tdbvalue = document().GetTDBValue(item_data->GetId());
  +        }
  +    table_adapter_->SetTDBValue(selected_tdbvalue);

When I see '->', I ask: what if it's NULL?
 - can we prove (maybe with some effort) that it can't be NULL?
 - can we see *at a glance* that it couldn't *possibly* be NULL?
 - can we see at a glance that it couldn't possibly *become* NULL
     here due to some future change elsewhere?
This variable is
    boost::shared_ptr<DatabaseTableAdapter> table_adapter_;
Now, smart pointers prevent memory leaks, and that's good.
And this one can extend an object's lifetime; is that valuable
here...or could scoped_ptr have been used instead? If it could,
then I'd consider storing it as a reference member. That would
prevent copying, but isn't this already a noncopyable class?
And it would show us where to enforce the desirable invariant
that this pointer be always dereferenceable--presumably in the

The table_adapter_ is shared by DatabaseView and a instance of
DatabaseEditorGrid which accepts boost::shared_ptr. We could add an
additional field to DatabaseView which would be a reference to
table_adapter_ but we still need to keep a shared_ptr (somewhere in a
pocket) only to ensure it is not deleted by DatabaseEditorGrid. I'd
add a pair of methods:
DatabaseTableAdapter& DatabaseView::table_adapter() { *table_adapter; }
+ the const version.

  inline MultiDimGrid& TreeGridViewBase::GetGridCtrl() const
      return *grid_;

When I see that, I automatically think of changing it this way:
 - Assert a non-NULL precondition.
 - Move it from the header to a '.cpp' file. The assertion
   would require including "assert_lmi.hpp", which in turn
   includes other headers of its own, so we should resist the
   urge to add the assertion in the header. I don't imagine
   there's much benefit to inlining anyway. I tend to avoid
   writing inline functions in headers these days, because
   changing the implementation can trigger a lot of recompiling,
   and I spend a lot of time recompiling already.

Oki. I'll modify the patch and resend a proper version to you asap.


reply via email to

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