Sign in to follow this  

Virtual function design question

This topic is 2956 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

There's a problem I've encounted several times and I've never been very happy with the solutions I've used, so I figured that I would see if anyone has a better solution. So I have a base class A:
class A
{
	virtual int func( )=0;
}
and then classes B-H that all derive from A and implement func(). I have some function that calls func using an A pointer, a vector of A pointers, or something else like this:
void somefunc()
{
	for ( i=0; i<avector.size( );i++ )
	{
		A* a=avector[i];
		int result=a->func( );
		//use result or something
	}
}
The problem occurs when I want to add a class I, but its result depends on some other data X within somefunc(). There are two ways I've solved this in the past: Using a type check to call I's func() differently:
void somefunc()
{
	X x;
	for ( i=0; i<avector.size( );i++ )
	{
		A* a=avector[i];
		int result=0;
		if ( a->isI( ) )
			result = static_cast<I*>( a )->func( x );
		else
			result = a->func();

		//use result or something
	}
}
or change A-H so that func() takes an X parameter, but B-H all just ignore it:
void somefunc()
{
	X x;
	for ( i=0; i<avector.size( );i++ )
	{
		A* a=avector[i];
		int result=a->func( x );
		//use result or something
	}
}
Both of these options work, but they feel ugly to me. Option 1 scales a little better if I end up having to add something like class J with func( Y y ) later. Option 2 is better from an OOP perspective, but it feels wrong to modify A-H just to make I work. Does this situation occur to anyone else? Do you solve it differently? Is there a better way to re-architect this? Thanks, Simon

Share this post


Link to post
Share on other sites
This generally means that the interface of A is not sufficient for what you're applying it to. It's harder to say more without additional detail and context, but a better solution is almost always to change the overall design -- changing the interface, adding a different one, changing how you operate on those interfaces, et cetera -- than to try and force an improper usage into the framework of the existing design.

Share this post


Link to post
Share on other sites
Quote:
Original post by scjohnno
I doesn't conform to the interface specified by A, so should I really be an A? Perhaps you can define a new interface, and have somefunc() operate on it separately.


Sorry, I should have made that clearer. func() is only a small part of the A class, imagine that there are a dozen other virtual functions that are overridden by the other classes, including I. The only thing unique about I is that one of the dozen functions on it needs a bit more data then any of the other classes requires. There's obviously some refactoring required, I'm just looking for a nicer way than the two I posted, if there is one.

Quote:
Original post by jpetrie
This generally means that the interface of A is not sufficient for what you're applying it to. It's harder to say more without additional detail and context, but a better solution is almost always to change the overall design -- changing the interface, adding a different one, changing how you operate on those interfaces, et cetera -- than to try and force an improper usage into the framework of the existing design.


That's pretty much the heart of my post I think. The second solution appears to be the most correct of the two, where the interface for A is changed and B-H are updated to use the new interface, but ignore the X argument. It gets a bit more complicated if another new class with a unique requirement is needed, like J::func( Y y ). In that case the first solution requires less work, but is uglier.

I'm 100% in favor of changing the overall design in order to deal with the new requirements for I(and potentially J), I just don't know what options there are other than the two I posted.

Quote:
Original post by Antheus
Instead of posting the solution - what was the problem?


The problem is that I need to add class I, but it has some additional requirements that A-H do not have. Is there a better way to refactor the hierarchy than the two ideas I posted?

Share this post


Link to post
Share on other sites
Quote:

The second solution appears to be the most correct of the two, where the interface for A is changed and B-H are updated to use the new interface, but ignore the X argument. It gets a bit more complicated if another new class with a unique requirement is needed, like J::func( Y y ). In that case the first solution requires less work, but is uglier.

The first option is almost never ideal. The second solution is better, but still subpar. The fact that one subclass needs extra data that the rest will ignore suggests that that subclass shouldn't be subclassed from the base. You can't treat it the same, so it isn't the same. But this depends on the actual problem you're trying to solve -- what I, X, A, B and J (et cetera) really represent.

Quote:

I'm 100% in favor of changing the overall design in order to deal with the new requirements for I(and potentially J), I just don't know what options there are other than the two I posted.

There are hundreds of thousands of them. Only a subset of them apply however -- it depends very much on the actual problems you're trying to solve. Your examples are too abstract to be of use to you.

Quote:

The problem is that I need to add class I, but it has some additional requirements that A-H do not have. Is there a better way to refactor the hierarchy than the two ideas I posted?

I think Antheus was getting at the same thing I am: we need some additional contextual information, some information about what these useless letters represent. This is an OO design problem, and to deal with it effectively you need to speak in the vocabulary of the objects.

Share this post


Link to post
Share on other sites
While waiting for more context...
Basically, class I needs some extra information via class X to fulfill the promise given by A::func(). Instead of pushing this information, let class I simply pull it.

class X {
private:
static X* last_created;
public:
static X* get_last_created()
{
if (last_created==0) throw PleasePanicNow("HAND");
return last_created;
}
X() { last_created=this;}
~X() { if (last_created==this) last_created=0;}
};

class I {
public:
int func()
{
X* x=X::get_last_created();
// do something
}
};

void somefunc()
{
X x;
for ( i=0; i<avector.size( );i++ )
{
A* a=avector[i];
int result=a->func();
/* mysterious stuff happens */
}
}


As far as I can see, this code has the same behaviour as yours - unless the constructor of X is called at an inopportune moment.

Share this post


Link to post
Share on other sites
Quote:
Original post by jpetrie
I think Antheus was getting at the same thing I am: we need some additional contextual information, some information about what these useless letters represent. This is an OO design problem, and to deal with it effectively you need to speak in the vocabulary of the objects.


Ok then if specifics are needed. The most recent example of this problem that I ran into was for the items in a shop. A is the base class for the item, and B-H are various different kinds of items. A has many different virtual functions that B-H all implement. The function func() from the example is:


virtual int getprice( Person* owner );



Each item type uses the owner and its own data to determine its price.

The new class that I'm trying to add(I) is an item type where the price depends on the time of day or the day of the week. In order to do that, the getprice() function needs to have access to that information.

Another example of a class(J) would be a type of item where I want some aspect of the buyer to influence the price, so getprice() would need access to that.

With those two new classes, my getprice() function either becomes:


virtual int getprice( Person* owner, Person* buyer, int dayofweek );



and all the other classes just ignore buyer and dayofweek, or I put the special cases at the point where I call getprice().

Obviously, I don't really like either of those options. If there's no other way to restructure things or set this scenario up I can accept that. I just hope there's some better way.

Thanks again,

Simon

Share this post


Link to post
Share on other sites
Quote:
Original post by RandomBystander
While waiting for more context...
Basically, class I needs some extra information via class X to fulfill the promise given by A::func(). Instead of pushing this information, let class I simply pull it.
*** Source Snippet Removed ***
As far as I can see, this code has the same behaviour as yours - unless the constructor of X is called at an inopportune moment.


Aside from being fairly WTF at first sight, that code isn't reentrable and probably is very hard to debug.

Share this post


Link to post
Share on other sites
While at first letting item determine its worth seems like a good idea, it really isn't. After all - when was the last time you asked your coffee cup how much it costs. The best item can provide is "worth", some abstract unit of its value.

In typical economy, actual price will be determined by owner and usually seller.

This means:
struct Seller {
virtual int getPrice(Item * i, Person * buyer);
};


What about other properties? Implementation detail.

Shopkeeper will be somehow placed in world, so they'll be able to know the time.
struct Shopkeeper : Person, Seller {
int getPrice(Item * i, Person * buyer) {
if (i->owner() != this) // not ours to sell
if (buyer->getFaction() != this->getFaction()) // we don't like them
int multiplier = 1;
if (this->getWorld()->getDay() == 2) multiplier = 2; // double price on Tuesday
return item->worth() * multiplier;
}
};
Or something similar.

Share this post


Link to post
Share on other sites
Quote:
Original post by Antheus
While at first letting item determine its worth seems like a good idea, it really isn't. After all - when was the last time you asked your coffee cup how much it costs. The best item can provide is "worth", some abstract unit of its value.

In typical economy, actual price will be determined by owner and usually seller.

This means:
struct Seller {
virtual int getPrice(Item * i, Person * buyer);
};


What about other properties? Implementation detail.

Shopkeeper will be somehow placed in world, so they'll be able to know the time.
struct Shopkeeper : Person, Seller {
int getPrice(Item * i, Person * buyer) {
if (i->owner() != this) // not ours to sell
if (buyer->getFaction() != this->getFaction()) // we don't like them
int multiplier = 1;
if (this->getWorld()->getDay() == 2) multiplier = 2; // double price on Tuesday
return item->worth() * multiplier;
}
};
Or something similar.


For explanation, the reason I implemented it with a virtual function and asking the item itself what it costs is to get away from something like:


switch (item->gettype( ))
{
case COFFEE: return X * Y;
case BREAD: return item->value( )*((istuesday( ))?2:1);
case JEWELRY: return ((buyer->shoesAreExpensive( ))?1000:500);
...etc...
}


I could certainly implement it in a way similar to your example, it would just require lots of type checks on the individual items like the above switch statement. It's my understanding(which may be completely wrong) that replacing explicit type checks with virtual functions is a better design? Or maybe I'm applying that principle in a place where it does not fit?

Share this post


Link to post
Share on other sites
Item costs make sense to keep in a table (like a catalog or menu).

Just create a std::map of itemID's to cost in a given Seller and call return price[item->get_type()]; or something similar to lookup prices.

From here, it is pretty simple to make this external data. Just populate the map by loading from a file or database.

Share this post


Link to post
Share on other sites
Quote:
switch (item->gettype( ))
{
case COFFEE: return X * Y;
case BREAD: return item->value( )*((istuesday( ))?2:1);
case JEWELRY: return ((buyer->shoesAreExpensive( ))?1000:500);
...etc...
}


Rather than having the price calculation asking for the item type to determine price modifiers, consider letting each item type describe what price modifiers it has. (The item doesn't even need to know they will be used in price calculations necessarily, depends on how general they can be.)

There are a number of options to go about that, the two on top of my head:

1) Virtual functions (base item class having e.g. isExtraExpensiveOnWeekday(int day) or something less price specific). The problem here is that there can be a rather large number of functions and changes all over the place - it'll be very exhausting to add a new price modifier.

2) Encapsulate price modifier information in a class of its own, and use a table (= vector or map) to look up price information depending on the type. As a benefit it's easy to load key/value data from external table file if you've already got a data description format in place. This is pretty much what Valere mentions, except that you can store more than just the item's worth (storing a tuple per entry rather than a single value).

struct ItemPriceModifiers {
int baseWorth;
DayEnum moreExpensiveOnThisDay;
bool dependsOnBuyersShoes;
};
//class above should probably have the fields private and only modified by constructor, I mad all fields nonconst/public for simplicity

std::unordered_map<int, ItemPriceModifiers*> itemPriceModifiers; //in some table class

How does this approach coincide with OO design? Well, I like to think that a class shouldn't try to do too much at once. It's always tempting to put alll functionality that depends on a class in that class, but you risk ending up with a bunch of loosely related inhomogeneous functions that way. (A classic example is whether a game entity should know how to render itself.)

[Edited by - Beyond_Repair on November 6, 2009 7:44:29 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by SimonFarris
The new class that I'm trying to add(I) is an item type where the price depends on the time of day or the day of the week.


So the first question you need to find the answer to is: why does the price of I depend on time, when the price of nothing else does?

Share this post


Link to post
Share on other sites
Quote:

So the first question you need to find the answer to is: why does the price of I depend on time, when the price of nothing else does?


It really ruins the informative experience for me when I come across holier-than-thou answers such as these. If you know something, sharing is a better way to show off!

Share this post


Link to post
Share on other sites
He's not trying to show off, he's trying to help. Besides, the OP knows his situation better than anyone else here, so he's the person in the best position to answer that question!

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
Quote:
Original post by SimonFarris
The new class that I'm trying to add(I) is an item type where the price depends on the time of day or the day of the week.

So the first question you need to find the answer to is: why does the price of I depend on time, when the price of nothing else does?

(Not being the OP, nor a learned economist, I try to answer anyway.)
Because buyer and seller have agreed that it does.
In other words, it doesn't.

It does not explicitly depend on time, that is. Explicitly, it only depends on the buyer and the seller, who negotiate the price. This negitiation always happens, even at the supermarket. The price label communicates an offer to sell the good at the stated price in household quantities, while there is any in stock, until further notice, and by carrying it to the cashier, you communicate that you agree to the terms.
Implicitly, however, there might be a sign saying that fruit and vegetables are half price after saturday noon.

Of course, the number on the price label does not depend on the customer, only the seller and his estimation of the market.

Short version: You do not ask the good itself about its price. Instead you look at the price label or, if there is none, ask the merchand selling it (which usually amounts to the same thing).

Share this post


Link to post
Share on other sites

This topic is 2956 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this