English 中文(简体)
Refactoring function pointers to some form of templating
原标题:

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.

最佳回答

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
问题回答

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.

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 function 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.





相关问题
Undefined reference

I m getting this linker error. I know a way around it, but it s bugging me because another part of the project s linking fine and it s designed almost identically. First, I have namespace LCD. Then I ...

C++ Equivalent of Tidy

Is there an equivalent to tidy for HTML code for C++? I have searched on the internet, but I find nothing but C++ wrappers for tidy, etc... I think the keyword tidy is what has me hung up. I am ...

Template Classes in C++ ... a required skill set?

I m new to C++ and am wondering how much time I should invest in learning how to implement template classes. Are they widely used in industry, or is this something I should move through quickly?

Print possible strings created from a Number

Given a 10 digit Telephone Number, we have to print all possible strings created from that. The mapping of the numbers is the one as exactly on a phone s keypad. i.e. for 1,0-> No Letter for 2->...

typedef ing STL wstring

Why is it when i do the following i get errors when relating to with wchar_t? namespace Foo { typedef std::wstring String; } Now i declare all my strings as Foo::String through out the program, ...

C# Marshal / Pinvoke CBitmap?

I cannot figure out how to marshal a C++ CBitmap to a C# Bitmap or Image class. My import looks like this: [DllImport(@"test.dll", CharSet = CharSet.Unicode)] public static extern IntPtr ...

Window iconification status via Xlib

Is it possible to check with the means of pure X11/Xlib only whether the given window is iconified/minimized, and, if it is, how?

热门标签