Click here to Skip to main content
15,868,141 members
Please Sign up or sign in to vote.
4.67/5 (3 votes)
Hi all,

I'm refactoring some ancient code following a move from VC6 to VS2005, and wonder if there's a way of tidying up the following situation:

There are two related but independent classes containing functions with identical names and parameters, call them ClassA and ClassB. The main code contains an instance of either ClassA or ClassB, and the code is littered with the following situation:
if (pClassA)
    pClassA->function();
else if (pClassB)
    pClassB->function();

If the two classes had the same base class I could do something like:
BaseClass* GetPointer()
{
    if ( pClassA ) return pClassA;
    else if ( pClassB ) return pClassB;
}

and in the main code do:
GetPointer()->function();

but they don't, so I can't.
So, your starter for ten is this: is there a way of streamlining this so I can remove the repeated if-else blocks and do something similar to the way the functions can be called if the classes have the same base class, but without adding a new common base class containing a virtual version of function()?
Posted

This does not make a sense at all. You might have missed something by any chance. Consider the following scenario based on what you said.

Class A {
  DoStuff1();
  DoStuff2();
  DoStuff3();
}

Class B {
  DoStuff1();
  DoStuff2();
  DoStuff3();
}


Now if I instantiate both classes

A *pClassA = new A();
B *pClassB = new B();


Tell me how can you call B's methods using pClassA? you can't. I think what the code is doing is it is checking the instance of A and B are valid. Of course you don't want to call if either pClassA or pClassB are null, or do you?

C#
if (pClassA)
    pClassA->function();
else if (pClassB)
    pClassB->function();



As far as refactoring the code, I am sure there could something that can be done. Are you familiar with design patterns? I suggest look into this Scott Meyers design pattern books http://www.amazon.com/Effective-Specific-Improve-Programs-Designs/dp/0321334876/ref=sr_1_5?s=books&ie=UTF8&qid=1300375803&sr=1-5[^] and http://www.amazon.com/More-Effective-Improve-Programs-Designs/dp/020163371X/ref=pd_bxgy_b_img_b[^]
 
Share this answer
 
Comments
hairy_hats 17-Mar-11 11:43am    
I'm not trying to call B's methods from A. As I said above, the code only instantiates one or the other. I am just looking for methods that allow me not to have to check if pClassA or pClassB have been allocated before each and every function call.
Yusuf 17-Mar-11 12:05pm    
So, if the code instantiates one or the other you will have either pClassA or pClassB, right?
hairy_hats 17-Mar-11 12:17pm    
Yes, so every time it calls one of the functions it has to check which one is instantiated.
Yusuf 17-Mar-11 13:06pm    
No, No, No. I think that is where you are getting it wrong.
Please see my second answer.
Espen Harlinn 18-Mar-11 6:20am    
Good points, my 5
Disclaimer: I don't like to post second answer unless it is radically different from the first. But in this case the discussion needs more space and explanation, so I am posting it as Answer.

You said: Yes, so every time it calls one of the functions it has to check which one is instantiated.

No, No, No. You are getting it wrong. checking with if(pClassA) or if(pClassB) does not tell you which type it is. It only checks if the pointer is valid (i.e it is instantianted, period)

Take this for example

A *pClassA = new A();
A *pClassB = new A();
B *pClassC = new B();

if (pClassA)
  //Do something
else if (pClassB)
  //Do different thing
else if (pClassC)
  // Do entirely different thing


Do you thing it is checking for the type there, No. It only validates it is instatiated. In other words it is equivalent to saying

if (null != pClassA)
  //Do something
else if (null != pClassB)
  //Do different thing
else if (null != pClassC)
  // Do entirely different thing


To check for type you have to call typeid

if (typeid(pClassA) == typeid(A*))

you see I doubt the original programmer is checking for the type, rather they are checking the object is instantiated and is not null.
 
Share this answer
 
Comments
#realJSOP 17-Mar-11 13:20pm    
He's still got the much maligned if statement to deal with.
Yusuf 17-Mar-11 13:32pm    
yea, but not the way he is thinking it. He things that if statement is checking the type, at least that is how I understood him
hairy_hats 17-Mar-11 16:42pm    
No, the type is known. pClassA is always of type ClassA and pClassB is always of type ClassB, but only one of them will be allocated.
Espen Harlinn 17-Mar-11 19:37pm    
Good effort, my 5
Sergey Alexandrovich Kryukov 17-Mar-11 23:21pm    
Yusuf, your explanation if quite correct, my 5.
However, a reasonable solution cab be a facade using wrapper class with the same interface, see my Answer.
--SA
If the classes share some method names (and their signatures), they, de facto, implement the same interface, hence I would explicitly create such an interface and make the two classes derive from it.
 
Share this answer
 
Comments
hairy_hats 17-Mar-11 16:48pm    
I would if it wasn't for MFC Serialization, which doesn't appreciate changing the derivation of the classes it is trying to load.
CPallini 17-Mar-11 17:06pm    
Well, just in serialization methods you may distinguish the classes, that would be, in my opinion, a reasonable trade-off.
Yusuf 17-Mar-11 16:49pm    
Well, Interface will do it, but in his case I think Abstract Base Class may be the solution. He use the base class methods without worrying which one is instantiated.
Espen Harlinn 18-Mar-11 6:21am    
Reasonable approach - 5ed!
First of all, when you say "I don't want don to make a common base class"… Frankly, this is the best you could do. However, if you find it difficult or want really non-intrusive way of using your legacy, I'll understand that: after all, legacy is legacy, some ugliness may be unavoidable.

You certainly should not check up the class every time. Even worse if you check by type comparison: it's slow and not supportable especially if you change inheritance in any way.

I suggest you use facade pattern. You can make a wrapper around your legacy classes. Here is how:

C++
class ABWrapper;

class ClassA {
friend class ABWrapper;
	void M() { /*...*/ }
}; //class ClassA

class ClassB {
friend class ABWrapper;
	void M() { /*...*/ }
}; //class ClassB

class ABWrapper {
public:
	ABWrapper(ClassA &a) : a(&a), b(0) {}
	ABWrapper(ClassB &b) : a(0), b(&b) {}
	//a and b cannot be null at the same time, due to constructors:
	void M() { a ? a->M(); : b->M(); }
private:
	ABWrapper() : a(), b() {} //to make this combination impossible
	ClassA *const a;
	ClassB *const b;
}; //class ABWrapper


Some comments on the code. I suggest you make the interface of your legacy classes private and use friend declaration instead. Even though the common rule of thumb is "don't use friends", dealing with the legacy is a valid excuse for using them. The code is use of passing a class instance by reference to the constructors used with explicitly inaccessible parameter-less constructor guarantees that one of the pointers to the instances of the class is not null.

The wrapper does not control life time if the instances and does not provide destruction. If the implementation where instantiation of the classes ClassA and ClassB is possible and desirable, destruction of the instances should be done in destructor of the wrapper.

—SA
 
Share this answer
 
v2
Comments
Espen Harlinn 18-Mar-11 6:21am    
Sound advice, my 5
Sergey Alexandrovich Kryukov 18-Mar-11 14:11pm    
Thank you, Espen. Even though OP argues against it, too, I don't think a miracle (i.e., a better way) exist.
--SA
hairy_hats 18-Mar-11 6:42am    
This method still involves doing a check on which class is instantiated at every method call {a ? a->M(); : b->M();} so effectively does the same as JSOP's original solution. If I have to do a comparison at every call, there isn't much to be gained and I might as well leave the code as it is.
Sergey Alexandrovich Kryukov 18-Mar-11 14:09pm    
You should understand: there is no such thing as miracle. This is the fastest theoretically possible method. Let's see.

1) You do not compare types, which is much slower; nothing is faster then comparison with 0: 1 CPU clock.

2) The CPU time used for finding a class in each call in no more than a regular virtual method dispatch using virtual table for indirection (in fact if this block is faster.

3) You never repeat if block anywhere in you code. This is the most important factor.

This is the solution; You can make something equivalent, but you won't be able to improve things.
I think, you should formally accept it.

--SA
CPallini 18-Mar-11 10:31am    
Very good, my 5.
Whay can't you just do this:

C++
public void CallFunction()
{
    if (pClassA)
    {
        pClassA->function();
    }
    else 
    {
        if (pClassB)    
        {
            pClassB->function();
        }
    }
}


This way, you always call the CallFunction method, and it hides the if/else nastiness from the programmer. You can then replace all of the existing if/else blocks with the call to CallFunction().
 
Share this answer
 
Comments
hairy_hats 17-Mar-11 10:33am    
It's a possibility, but I'm hoping to find a way which avoids having to do the comparison every time. I'm looking at Boost.Function as a possible way of setting up a table of pointers to functions at the top of the program, so no comparison needs to be made at the point of calling, but it's crude in comparison to polymorphism.
#realJSOP 17-Mar-11 11:59am    
Well, I think the maintainability of the single function that I provided is far and away more important than doing something that requires you to create a table of pointers too functions. I'm not entirely sure that you're going to get away from the if statement. It looks like classA could or could not be instantiated, and if it it isn't, you then check classB. I don't see how you can get away from the if statement.
hairy_hats 17-Mar-11 12:05pm    
You can't get away from the if statement completely, but it could be possible to just do it once at the top of the program where a table would be set up. It wouldn't then be necessary to do it every time, just call the bound functions directly. It could be that the most efficient way would be to only bother binding the most commonly-used functions, and do a single function like the one you proposed for the rarer ones.
#realJSOP 17-Mar-11 13:19pm    
I think you miss the significance of the if statement. It appears as if it has to be checked each time you want to call the function. Of course, I'm just guessing becausae we don't know the particulars. With my solution, the if onbly happens in one place. Besides, every place that used to have the if statement can now just call the new function, and the if statement is in just one place.
hairy_hats 18-Mar-11 10:01am    
You win for simplicity, John.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900