Refactoring function pointers to some form of templating

Question

Bear with me as I dump the following simplified code: (I will describe the problem below.)

class CMyClass
{
    ...
private:
 HRESULT ReadAlpha(PROPVARIANT* pPropVariant, SomeLib::Base *b);
 HRESULT ReadBeta(PROPVARIANT* pPropVariant, SomeLib::Base *b);

 typedef HRESULT (CMyClass::*ReadSignature)(PROPVARIANT* pPropVariant, SomeLib::Base *b);

 HRESULT TryFormats(ReadSignature ReadFormat, PROPVARIANT* pPropVariant);
};


inline HRESULT CMyClass::ReadAlpha(PROPVARIANT* pPropVariant, SomeLib::Base *b)
{
 if (b)
 {
     // got a valid Base. Handle generic stuff here.
     SetStuff(pPropVariant, b->someInt);
     return S_OK;
 }

 return (b != NULL) ? 0 : -1;
}

inline HRESULT CMyClass::ReadBeta(PROPVARIANT* pPropVariant, SomeLib::Base *b)
{
 if (b)
 {
  SomeLib::FormatA *fa;
  SomeLib::FormatB *fb;

  if ( fa = dynamic_cast<SomeLib::FormatA*>( b ) )
  {
   // specific code for FormatA
   SetStuff(pPropVariant, fa->getVersion());
   return S_OK;
  }
  else if ( fb = dynamic_cast<SomeLib::FormatB*>( b ) )
  {
   // specific code for FormatB
   SetStuff(pPropVariant, fb->valueForB);
   return S_OK;
  }
 }

 return (b != NULL) ? 0 : -1;
}

inline HRESULT CMyClass::TryFormats(ReadSignature ReadFormat, PROPVARIANT* pPropVariant)
{
 HRESULT hr;
 if (FAILED(hr = (this->*ReadFormat)(pPropVariant, _pFile->formatA())))
  if (FAILED(hr = (this->*ReadFormat)(pPropVariant, _pFile->formatC())))
   hr = (this->*ReadFormat)(pPropVariant, _pFile->formatD());

 return hr;
}

I end up calling this code like:

hr = TryFormats(&CMyClass::ReadAlpha, pPropVar);

Now... the problem is that this is too generic and constrained, especially now that I am trying to refactor this code for use in some other projects. So, this means that I want to place the ReadXxx code in another source file and abuse templating somehow. The TryFormats remains in the class since different classes have different formats they attempt to read.

My current approach is bound to fail due to the dynamic_cast<Derived*> needed for functionality that is not in the Base class, and since I may need to read up to 5 different formats in one class, I really don't want to drag in formats I do not need in the first place. (For example, see above how CMyClass does not support SomeLib::FormatB, yet the ReadBeta() needs to support it and thus forces the compiler to compile all the relevant information in.) In total, I have around 10 different formats I 'support' like this.

How can I properly refactor this code? I don't want to need to rewrite Base functionality for every descendant, nor do I want to put derived specific information into a function that simply takes a Base.

I have tried some things, but all I manage to squeeze out of my compiler are rainbows of errors. Rather than confuse the people here with my attempts, I figured I'd give my (simplified) original working code and allow the experts to draw their own conclusions on how to do this. In reality, there's about 50 of those ReadXxx functions, but they either follow the general structure of the ReadAlpha or ReadBeta functions above. So if someone can show me how to do those, I can without an issue convert my actual code. (I imagine I will need to change TryFormats() definition as well, and that's no problem either - I am just hoping someone can show me how to do get the above example refactored properly.)

Thank you, and my apologies for the long, long question.

Answer 1

Updated from comment I would wrap SomeLib::Base in an adapter under your control. Give it 2 [pure] virtual methods one whos purpose is to provide the second argument to SetStuff, and second to return if a given method(?) -- ie alpha/beta -- is supported. And then also provide access to the underlying SomeLib:: class.

 class BaseAdapter
 {
 ...
 private:
     SomeLib::Base* m_concreteBase;
 public:
     virtual int SecondArgument(...) = 0; 
     virtual bool IsSupported(...) { return false;}

     SomeLib::Base* GetConcreteBase() {return m_concreteBase;}
 };

 class FormatAAdapter : public BaseAdapter
 {
     ...
     int SecondArgument(alphaOrBetaOrWhatever)
     {
         // return based on source function
     }

     bool IsSupported( alphaOrBetaOrWhatever )
     {
         // return true/false based on source function
     }
 }

 // Create a funtion to create one of each type of format, ensuring type safety
 virtual BaseAdapter* MakeBaseAdapter(SomeLib::FormatA* fa)
 {
        return new FormatAAdapter(fa)
 }

Then instead of

  SomeLib::FormatA *fa;
  SomeLib::FormatB *fb;

  if ( fa = dynamic_cast<SomeLib::FormatA*>( b ) )
  {
   // specific code for FormatA
   SetStuff(pPropVariant, fa->getVersion());
   return S_OK;
  }
  else if ( fb = dynamic_cast<SomeLib::FormatB*>( b ) )
  {
   // specific code for FormatB
   SetStuff(pPropVariant, fb->valueForB);
   return S_OK;
  }

You could do

 ReadBeta(PROPVARIANT* pPropVariant, BaseAdapter *b)
 {

    // specific code for FormatB
    if (b->IsSupported(beta))
    {
       SetStuff(pPropVariant, b->SecondArgument(beta));
       return S_OK;
    }
 }

In your calling code you would work through your adapters:

inline HRESULT CMyClass::TryFormats(ReadSignature ReadFormat, PROPVARIANT* pPropVariant)
{
 HRESULT hr;
 if (FAILED(hr = (this->*ReadFormat)(pPropVariant, MakeBaseAdapter(_pFile->formatA())))
  if (FAILED(hr = (this->*ReadFormat)(pPropVariant, MakeBaseAdapter(_pFile->formatC()))))
   hr = (this->*ReadFormat)(pPropVariant, MakeBaseAdapter(_pFile->formatD()));

 return hr;
}

Also, in response to

A good deal of Base descendants would not support that specific secondArgument, and if they did, it might be calculated. Using #IFDEFs would be a cleaner solution (but I prefer templates!)

You can either provide a default value for the secondArgument or provide a way through the base adapter to notify the user that the secondArgument is not available.

As an aside, when I hear "Refactoring function pointers to some form of templating" I think boost functions.

Answer 2

Sorry for a long post.
One possible solution is to implement visitor pattern. Unfortunately it requires one time modification to you SomeLib, but after that you can expand its functionality without further modifications. In fact Visitor is a framework that supports Open/Close principle. Implement it once and you will be able to add functionality to your library without actual modification to the library itself.

Here is implementation sketch:
Declare new class in your SomeLib:

// visitor base class, acts as interface can not be instanciated.
// must be delared in SomeLib
class IVisitor
{
protected:
 IVisitor()   {}

public:
 // for all supported formats
 virtual HRESULT  OnVisitFormatA(SomeLib::FormatA& formatA) 
                                                       {return NoOperation();}
 virtual HRESULT  OnVisitFormatB(SomeLib::FormatB& formatB) 
                                                       {return NoOperation();}

private:
 HRESULT    NoOperation() {return 0;}
};

Every class in you SomeLib::base hierarchy must implement new virtual function:

virtual HRESULT Accept(IVisitor& visitor);

Implementation of Accept will be class specific:

HRESULT  FormatA::Accept(IVisitor& visitor)
{
 return visitor.OnVisitFormatA(*this);
}

HRESULT  FormatB::Accept(IVisitor& visitor)
{
 return visitor.OnVisitFormatB(*this);
}

Now we are done with changes to SomeLib Lets move to your application.
First we need to implement concrete visitor classes:

class CMyClass; // forward delare
class Visitor : public SomeLib::IVisitor
{
protected:
 Visitor(CMyClass* myclass, PROPVARIANT* propvariant) 
         : myclass_(myclass), propvariant_(propvariant)
 {
 };

protected:
 CMyClass* myclass_;
 PROPVARIANT* propvariant_
};

This is still non-instanciable class.
Now we need concrete classes for whatever read you happen to need.

class ReadAlphaVisitor : Visitor
{
public:
 ReadAlphaVisitor(CMyClass* myclass, PROPVARIANT* propvariant) 
            : Visitor(myclass, propvariant)
 {
 }

public:
 virtual HRESULT  OnVisitFormatA(SomeLib::FormatA& formatA)
                                                    {return ReadAlpha(formatA);}
 virtual HRESULT  OnVisitFormatB(SomeLib::FormatB& formatB) 
                                                    {return ReadAlpha(formatB);}

private:
 HRESULT   ReadAlpha(SomeLib::base& format)
 {
  myclass_->SetStuff(propvariant_, format.someInt);
  return S_OK;
 }
};

And another one:

class ReadBetaVisitor : Visitor
{
public:
 ReadBetaVisitor(CMyClass* myclass, PROPVARIANT* propvariant) 
                 : Visitor(myclass, propvariant)
 {
 }

public:
 virtual HRESULT  OnVisitFormatA(SomeLib::FormatA& formatA) 
                                                 {return ReadBetaFormatA(formatA);}
 virtual HRESULT  OnVisitFormatB(SomeLib::FormatB& formatB) 
                                                 {return ReadBetaFormatB(formatB);}

private:
 HRESULT   ReadBetaFormatA(SomeLib::FormatA& formatA)
 {
  myclass_->SetStuff(propvariant_, formatA.getVersion());
  return S_OK;
 }

 HRESULT   ReadBetaFormatB(SomeLib::FormatA& formatB)
 {
  myclass_->SetStuff(propvariant_, formatB.valueForB);
  return S_OK;
 }
};

And finally here is how MyClass will use them:

inline HRESULT CMyClass::ReadAlpha(PROPVARIANT* pPropVariant, SomeLib::Base *b)
{
 if( 0 != b )
 {
  ReadAlphaVisitor visitor(this, pPropVariant);
  return b->Accept(visitor);
 }

 return 0;
}

inline HRESULT CMyClass::ReadBeta(PROPVARIANT* pPropVariant, SomeLib::Base *b)
{
 if( 0 != b )
 {
  ReadBetaVisitor visitor(this, pPropVariant);
  return b->Accept(visitor);
 }

 return 0;
}

It scares me just to look at it :-)
It may be terribly overdesigned, but still a good exercise.

Update: To avoid including all the formats IVisitor can be redefined as follows:

class IVisitor
{
protected:
 IVisitor()   {}

public:
 // for all supported formats
 virtual HRESULT  OnVisitFormatA(SomeLib::base& formatA) 
                                                       {return NoOperation();}
 virtual HRESULT  OnVisitFormatB(SomeLib::base& formatB) 
                                                       {return NoOperation();}

private:
 HRESULT    NoOperation() {return 0;}
};

Then application that uses your lib would implement visitor and override only what is needed (OnVisitFormatA only), but then of course downcasting is involved (argh...) and we are back at the drawing board, this design does not avoid downcasting and goes into trash bin.

Answer 3

Ok, my previous visitor approach is a history. I am going to post you entire text of small working program that you can play with. Assuming that

_pFile->formatA()
_pFile->formatC()
_pFile->formatD()

All declared as

FormatA* formatA()
FormatC* formatC()
FormatD* formatD()

In other words return type is known at compile time, this templatized approach may work for you. And it involves neither function pointers nor dynamic downcasting

//////////////////////////////////////////////////////////////////
// this section is for testing
class   Base 
{
public:
    void ExecuteBase()
    {
     cout << "Executing Base" << endl;
    }
};

class   FormatA : public Base
{
public:
    void ExecuteAAlpha()
    {
     cout << "Executing A Alpha" << endl;
    }

    void ExecuteABeta()
    {
     cout << "Executing A Beta" << endl;
    }
};

class   FormatB : public Base
{
public:
    void ExecuteBAlpha()
    {
     cout << "Executing B Alpha" << endl;
    }

    void ExecuteBBeta()
    {
     cout << "Executing B Beta" << endl;
    }
};

FormatA* GetFormatA()
{
    static FormatA cl;
    return &cl;
}

FormatB* GetFormatB()
{
    static FormatB cl;
    return &cl;
}
//////////////////////////////////////////////////////////////////




//////////////////////////////////////////////////////////////////
// now begins real code
struct AlphaReader  {};
struct BetaReader {};
template <typename READER_TYPE> struct TypeConverter {};


class   MyClass
{
public:
    template <typename READER_TYPE>
    int TryFormats(const READER_TYPE&)
    {
     TryFormatsImplementation(TypeConverter<READER_TYPE>(), GetFormatA());
     TryFormatsImplementation(TypeConverter<READER_TYPE>(), GetFormatB());

     return 0;
    }

private:
    int  TryFormatsImplementation(const TypeConverter<AlphaReader>&, Base* pFormat)
    {
     // here you will call you ReadAlpha which requires Base only
     // code below is for testing

     cout << "Executing Alpha Reader for Base" <<endl;
     pFormat->ExecuteBase();
     return 1;
    }

    int  TryFormatsImplementation(const TypeConverter<BetaReader>&, FormatA* pFormat)
    {
     // here you will call you ReadBeta for FromatA,
     // code below is for testing

     cout << "Executing Beta Reader for FormatA" <<endl;
     pFormat->ExecuteABeta();
     return 3;
    }

    int  TryFormatsImplementation(const TypeConverter<BetaReader>&, FormatB* pFormat)
    {
     // here you will call you ReadBeta for FromatB,
     // code below is for testing

     cout << "Executing Beta Reader for FormatB" <<endl;
     pFormat->ExecuteBBeta();
     return 4;
    }
};


int main()
{
    MyClass cl;

    cl.TryFormats(AlphaReader());
    cl.TryFormats(BetaReader());

    cin.get();
}

After I run this program I get following output which is correct:

Executing Alpha Reader for Base
Executing Base
Executing Alpha Reader for Base
Executing Base
Executing Beta Reader for FormatA
Executing A Beta
Executing Beta Reader for FormatB
Executing B Beta