CODEWITHSUNDEEP
c++refactoring
ossandcad
ossandcad

Reputation: 540

Refactor the following two C++ methods to move out duplicate code

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)

Upvotes: 8

Views: 1601

Answers (6)

Yan Tatarintsev
Yan Tatarintsev

Reputation: 1

O! Please, don't use the bugged statement:

return params.size() != 0;

It returns (-1) (all bits raised) since params is empty, not (1).

But you really can consolidate the return statement like this:

return (params.size() != 0) ? 0 : 1;

It seems right for the classes SURFACE and CURVE to be derived from the abstract class FIGURE or SHAPE or some one more. So you can escape from the template and use the virtual overriding:

void getParameters(const FIGURE& surface, VARIANT& varParams)

Upvotes: 0

rldrenth
rldrenth

Reputation: 35

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;
}

Upvotes: 0

Bj&#246;rn Pollex
Bj&#246;rn Pollex

Reputation: 76876

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.

Upvotes: 2

Narfanator
Narfanator

Reputation: 5813

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)
}

Upvotes: -2

GManNickG
GManNickG

Reputation: 504303

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

Upvotes: 11

Carl Manaster
Carl Manaster

Reputation: 40356

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.

Upvotes: 5

Related Questions