views:

170

answers:

5

I have the following two methods that (as you can see) are similar in most of its statements except for one (see below for details)

unsigned int CSWX::getLineParameters(const SURFACE & surface, vector<double> & params)
{
    VARIANT varParams;

    surface->getPlaneParams(varParams); // this is the line of code that is different

    SafeDoubleArray sdParams(varParams);

    for( int i = 0 ;  i < sdParams.getSize() ; ++i )
    {
        params.push_back(sdParams[i]);
    }

    if( params.size() > 0 ) return 0;
    return 1;
}

unsigned int CSWX::getPlaneParameters(const CURVE & curve, vector<double> & params)
{
    VARIANT varParams;

    curve->get_LineParams(varParams); // this is the line of code that is different

    SafeDoubleArray sdParams(varParams);

    for( int i = 0 ;  i < sdParams.getSize() ; ++i )
    {
        params.push_back(sdParams[i]);
    }

    if( params.size() > 0 ) return 0;
    return 1;
}

Is there any technique that I can use to move the common lines of code of the two methods out to a separate method, that could be called from the two variations - OR - possibly combine the two methods to a single method?

The following are the restrictions:

  1. The classes SURFACE and CURVE are from 3rd party libraries and hence unmodifiable. (If it helps they are both derived from IDispatch)
  2. There are even more similar classes (e.g. FACE) that could fit into this "template" (not C++ template, just the flow of lines of code)

I know the following could (possibly?) be implemented as solutions but am really hoping there is a better solution:

  1. I could add a 3rd parameter to the 2 methods - e.g. an enum - that identifies the 1st parameter (e.g. enum::input_type_surface, enum::input_type_curve)
  2. I could pass in an IDispatch and try dynamic_cast<> and test which cast is NON_NULL and do an if-else to call the right method (e.g. getPlaneParams() vs. get_LineParams())

The following is not a restriction but would be a requirement because of my teammates resistance:

  1. Not implement a new class that inherits from SURFACE/CURVE etc. (They would much prefer to solve it using the enum solution I stated above)
+2  A: 

Why not just pass the VARIANT varParams as a parameter to the function instead of a CURVE or a SURFACE?

unsigned int CSWX::getParameters(VARIANT varParams, vector<double> & params)
{
    SafeDoubleArray sdParams(varParams);

    for( int i = 0 ;  i < sdParams.getSize() ; ++i )
    {
        params.push_back(sdParams[i]);
    }

    if( params.size() > 0 ) return 0;
    return 1;
}

unsigned int CSWX::getPlaneParameters(const CURVE & curve, vector<double> & params)
{
    VARIANT varParams;    

    curve->get_LineParams(varParams); // this is the line of code that is different

    return getParameters( varParams, params );
}

You may also consider (if possible) making those methods templates and receive an output_iterator as parameter instead of a vector. This way your code does not depend on the kind of collection used.

Space_C0wb0y
How would that help?
Dave
I'm sorry. I didn't quite understand what you are suggesting. The methods called by 'surface' and 'curve' are different - which is the line of code that is different between the two methods. And varParams is a local variable that is not needed later. Perhaps I need to restate my question if not properly asked? Could you perhaps provide some sample code to help me understand?
ossandcad
Added code for clarity. This solution would make it shorter, especially if more methods are affected.
Space_C0wb0y
Likely means make a SafeDoubleArrayToVectorOfDouble method, that they both can call to do the conversion work.
sdg
+5  A: 

Extract Method. Everything after the lines you have marked as different is identical - so extract it as a method which is called from both of your original methods.

Carl Manaster
True - as much common code as possible to a separate method. This works. But does that mean, if I have more common code before the "different line of code", then "extracting" it would be difficult?
ossandcad
Basically, any code block that's duplicated can be extracted into its own method, regardless of how many lines lead up to it, or follow it. It can get messy, if the extracted method requires too many parameters, for instance, and alternative refactorings may sometimes be more appropriate. But Extract Method is a good place to start.
Carl Manaster
You'd have to do Extract Method twice (or more, depending on how many places you have non-identical code). Yes, it could get difficult at that point.
LH
+10  A: 

A couple ideas come to mind, but here's what I think would be best:

namespace detail
{
    void getParameters(const SURFACE& surface, VARIANT& varParams)
    {
        surface->getPlaneParams(varParams);
    }

    void getParameters(const CURVE& curve, VARIANT& varParams)
    {
        curve->get_LineParams(varParams);
    }
}

template <typename T>
unsigned int getParameters(const T& curve, vector<double> & params)
{
    VARIANT varParams;
    detail::getParameters(curve, varParams);

    SafeDoubleArray sdParams(varParams);
    for( int i = 0 ;  i < sdParams.getSize() ; ++i )
    {
        params.push_back(sdParams[i]);
    }

    return params.size() != 0;
}

What you do is delegate the task of getting parameters to some other function that is overloaded. Just add functions like that for each different type you have. (Note, I simplified your return statement.)

GMan
Nice. I like this. Am leaning towards this. Sad I didn't think of this myself.
ossandcad
Very nice, especially if combined with the `Extract Method` mentionned by Carl Manaster to move out some code (after the call to `detail::getParameters`). I like it better when my template methods are light: makes for lighter dependency graph :)
Matthieu M.
A: 

Instead of passing in a SURFACE or CURVE, pass in a reference to the base class, and also a method function pointer. Then the call to surface->getLine_parameters or curve->getPlaneParamters would be replaced by (shape->*getParamters)(varParams);

typedef void Baseclass::*ParamGetter(VARIANT varParams);

unsigned int CSWX::getLineParameters(const Baseclass & geometry, ParamGetter getParams
       vector<double> & params)
{
  VARIANT varParams;

  (geometry->*getParams)(varParams); // this is the line of code that is different

  SafeDoubleArray sdParams(varParams);

  for( int i = 0 ;  i < sdParams.getSize() ; ++i )
  {
      params.push_back(sdParams[i]);
  }

  if( params.size() > 0 ) return 0;
  return 1;
}
rldrenth
A: 

Macro! Usually not the best solution (it's a macro) and probably not the actual best in this one, but it'll do the job.

macro_GetDakine_Params(func)
    VARIANT varParams; \
    curve->##func(varParams); // this is the line of code that is different \
    SafeDoubleArray sdParams(varParams); \
    for( int i = 0 ;  i < sdParams.getSize() ; ++i ) \
    { \
        params.push_back(sdParams[i]); \
    } \
    if( params.size() > 0 ) return 0; \
    return 1; \

unsigned int CSWX::getPlaneParameters(const CURVE & curve, vector<double> & params)
{
    macro_GetDakine_Params(getPlaneParams)
}
unsigned int CSWX::getLineParameters(const CURVE & curve, vector<double> & params)
{
    macro_GetDakine_Params(getLineParams)
}
Narfanator