C++ and derived classes issue

[John Ralls]

> On Feb 13, 2016, at 8:02 AM, Geert Janssens <geert.gnucash at kobaltwit.be> wrote:
> 
> On Friday 12 February 2016 17:15:41 John Ralls wrote:
> > > On Feb 12, 2016, at 1:09 PM, Geert Janssens
> > I think that what you really want to do is
> > 
> > template <typename T> class GncTransProperty {
> > public:
> >   GncTransProperty (T x, GncTransPropType p) : value{x}, property{p}
> > {}; T get_value() const { return value; }
> >   GncTransPropType get_property() const { return property; }
> > 
> > private:
> >   T value;
> >   GncTransPropType property;
> > }
> > 
> > That does everything you want in 10 lines and avoids the evil void*.
> > Even better (at least until Transaction is converted to C++) would be
> > to expand its property list to include all of the things that the
> > importer will set. Then you can do even more, and get rid of the
> > second switch statement:
> > 
> > template <typename T> class GncTransProperty {
> > public:
> >   GncTransProperty (T x, const char* p) : value{x}, property{p} {};
> >   void set_trans_property (Transaction *txn) const {
> > 	GValue val = G_VALUE_INIT;
> > 	g_value_init_from_instance (&val, G_POINTER(value));
> > 	g_object_set_property (G_OBJECT (txn), property, &val);
> >  }
> > 
> > private:
> >   T value;
> >   char *property;
> > }
> > 
> > You'll have to specialize the set_txn_property implementation for
> > string, time64, and probably gnc_numeric since they're not GObjects.
> > The specialization code looks like:
> > 
> > template <> class GncTransProperty <std::string>{
> > public:
> >   GncTransProperty (std::string& x, const char* p) : value{x},
> > property{p} {}; void set_trans_property (Transaction *txn) const {
> > 	GValue val = G_VALUE_INIT;
> > 	g_value_set_string (&val, value.c_str());
> > 	g_object_set_property (G_OBJECT (txn), property, &val);
> >  }
> > 
> > private:
> >   std::string value;
> >   char* property;
> > }
> > 
> > With that and the changes in Transaction.c, the loop in
> > GncTransPropertyList::to_trans() can be reduced to
> > std::for_each(this->begin(), this->end(), [](auto
> > elem){elem.set_trans_property();});
> > 
> > BTW, except for the lambda and the curly-bracket initializers that's
> > all C++98.
> > 
> > Note that replacing get_type and get_value with set_trans_property
> > would work with a class hierarchy, too, because it gets rid of the
> > return-type problem with get_value. The template version runs
> > slightly faster because the functions aren't called by vtable. The
> > template version is also more flexible: The base template can handle
> > any GObject type without specialization. It should also be possible
> > once Transaction is changed to C++ to create a more type safe (never
> > mind faster) way to handle properties to get rid of the
> > specializations too because we should be able to say
> > txn->set(property, value).
> > 
> > Regards,
> > John Ralls
>  
> Thanks for the detailed explanation John. It took me several readings to grok it.
>  
> I certainly like the idea of a set_trans_property to avoid different return types.
>  
> I had already attempted to use a template class instead, but ran into several issues (partly due my long hiatus in c++ coding).
>  
> I'm redoing it now based on your suggestions. Current code is here:
> https://github.com/gjanssens/gnucash/blob/cpp/src/import-export/csv-imp/gnc-trans-props.cpp
> and
> https://github.com/gjanssens/gnucash/blob/cpp/src/import-export/csv-imp/gnc-trans-props.hpp
> 
> I'm still having a couple of issues:
>  
> 1. The first is mainly lack of knowledge. I note you write a constructor like this:
> GncTransProperty (T x, const char* p) : value{x}, property{p} {};
>  
> I'm not familiar with the ": value{x}, property{p}" notation. Is that a specific C++ notation to initialize private members ? Any caveats with it ? (I didn't come across an explanation in Meyers book so far, so I assume it's standard C++). Does it have a name I can google ?

I mentioned it in the line about C++11 features: It's called "curly-brace initializers" and it's discussed at length in Item 7 of "Effective Modern C++".

>  
> 2. I also note you are using "T x" in the constructor, suggesting the GncTransProperty is constructed from the properly typed value. However the importer so far only has a bunch of strings to process. The idea was exactly to let the GncTransproperty instances handle type conversion from the generic string to the proper internal type for later use. In this way that knowledge is encapsulated in the object that needs to know. If I forgo this, the calling code has to do the conversion, breaking the encapsulation and in fact removing much of the motivation to create the GncTransProperty class in the first place. So I have for now kept the more generic constructor
> GncTransProperty (std::string val, GncTransPropType p)
> This required specializations for each subtype of GncTransProperty we care about.

That's what the factory pattern is for. I saw that you already had a factory function so I figured I didn't need to go into that… it won't work with templated GncTransProperty types any more than std::vector will, but it will work fine with std::unique_ptr<GncTransProperty>.  Why do you want to construct the GncTransProperty before you know what you're going to put in it?

>  
>  
> 3. Just so you can follow I'll also note that I can't do this:
> template <typename T> class GncTransProperty
> ...
> std::vector<GncTransProperty> vctp;
> Based on the following StackOverflow topic,
> http://stackoverflow.com/questions/16527673/c-one-stdvector-containing-template-class-of-multiple-types
> I have hence chosen to write a pure virtual interface class GncTransProperty which is the base class of the GncTransPropImpl template class.
>  
> With this I can define
> std::vector<std::unique_ptr<GncTransProperty>> vctp
> which is the base class of GncTransPropertyList
> and get on with my life.

Right decision. Templatizing the subclasses is an interesting approach but since there's not really any "general" case—everything is a specializaiont—it's not clear that you gain anything.

> 4. Back to the real issues: if I would use g_object_set to set the date_posted for a transaction it will call different functionality inside the Transaction code than the importer is doing currently. g_object_set will eventually call xaccTransSetDatePostedTS where the importer has been calling xaccTransSetDatePostedSecsNormalized. The latter also sets a KVP (with the gdate only) where the former doesn't. I'm not sure if we really need that additional KVP, but don't like to pull too many strings at once either.

Christian set up that KVP as a fall-back to deal with the date moving around with timezones. I think it's used somewhere, but making the transaction time 1300Z is a better fix. Modulo the two or three users who want to be able to set posted times instead of just dates, switching to a julian-date representation would be even better, though it would create a file-compatibility issue.
>  
> 3. You propose to make all the possible import items properties of the Transaction GObject. I'm not really in favor of that as several of these import items are actually properties of splits to create. The name "Transaction" is used fairly liberally in the importer to mean a Transaction (in the engine sense of the word) together with its Splits. I don't think it's appropriate to add all of those into the Transaction engine object. On the other hand, having both a set_trans_property and a set_split_property again breaks the encapsulation. The calling object needs to know when to use which.

Hmm. 
>  
> 4. And it gets worse. There is the 'num' property of transactions. Depending on a user preference, this can also be stored in a split instead. So if the import data has a 'num' item, I can't just call
> g_object_set (trans, "num", &val) on it, because it may have to be set for a split instead.
>  
> 5. And lastly there's no direct mapping between the "Debit", "Credit" and "Balance" import types on the one hand and the split amount/value on the other.
> If the import data has both a Debit and a Credit column the split's amount should be Debit-Credit. If it has a Balance column instead, a whole different calculation will be done later on in the generic importer code.
>  
>  
> I'm beginning to wonder whether using a separate class to characterize just one property is actually the right approach…

You started out trying to encapsulate an object and a flag that indicated where to put it. That had genericity problems because there's no type-safe way to generically handle the differing return types. Polymorphism to the rescue! Except that it turns out that you have different handling requirements depending not on the type that you're storing but on the actual property that you're processing.

How about this:
template <typename T, GncTransPropType p> class GncTransPropImpl {
public:
    GncTransPropImpl (T v) : value {v} {}
   ...
    friend template<>void set_property(std::unique_ptr<GncTransPropImpl<T, P>> prop, Transaction *txn);
    friend template<>void set_property(std::unique_ptr<GncTransPropImpl<T, P>> prop, Split *split);
private:
    T value;
}

template<typename T, GncTransPropType P>void  set_property(std::unique_ptr<GncTransPropImpl<T, P>> prop, Transaction *txn) {}
template<typename T, GncTransPropType P>void  set_property(std::unique_ptr<GncTransPropImpl<T, P>> prop, Split *split) {}

template<> void set_property<Account *, GncTransPropImpl::Account>
(std::unique_ptr<GncTransPropImpl<Account*, GncTransPropImpl::Account>> prop,  Transaction *txn) {
  xaccTransactionSetAccount(txn, prop->value);
}

and so on? You could also do it more directly with set_property directly as a member function. That would require specializing the class explicitly for both parameters in the same way that set_property<>() is above.  The signature of the function can be different for different specializations to take a split, a txn, or both as needed. It just means there's more copying of all of the other class boilerplate. OTOH I'm sure that the class specialization will work, less sure that the friend-function version will.

Regards,
John Ralls