🔒 Locked

Class Operator Problem

Started by GamerAlan Feb 22, 2003 at 1:08 AM 10 replies 500+ views

Original Post

Feb 22, 2003 at 1:08 AM

Okay, the below program is an experiment in using classes. I create a class named CPlayer and give it a few data members and methods. I also create an operator, in atempt to learn how to use operators correctly, and that part doesn't work. (Evidently my book didn't cover it well enough!) The operator should sum the values of the level data member and reasign it to the current class object. Currently my program gives an illegal operation error which the debugger identifies as an "Expression: _CrtlsValidHeapPointer(pUserData) Heres the code I'm using:

    #include <iostream>
#include <string.h>

class CPlayer //creating the class

{
private:
	char *name;
	unsigned short level;  //the problem member data

	unsigned short strength;
public:
	CPlayer(void);
	CPlayer(unsigned short lvl, unsigned short str);
	CPlayer(CPlayer & copyPlayer);
	~CPlayer(void);
	CPlayer operator + (CPlayer & otherPlayer); //the operator that doesn't want to work

	char * GetName();
	void LevelUp();
	int GetLevel();
	void Fight();
};
CPlayer::CPlayer(void)
{
	level = 0;
}


CPlayer::CPlayer(unsigned short lvl, unsigned short str)
{
	level = lvl;
	strength = str;
	std::cout << "Enter your character name: ";
	char tempName[1024];
	std::cin >> tempName;
	int nameLength = strlen(tempName);
	name = new char[nameLength + 1];
	strcpy(name, tempName);
}

CPlayer::CPlayer(CPlayer & copyPlayer)
{
	level = copyPlayer.level;
	strength = copyPlayer.strength;
	//char tempName[1024] = copyPlayer.GetName();

	char tempName[1024];
	strcpy(tempName, copyPlayer.GetName());
	int nameLength = strlen(tempName);
	name = new char[nameLength + 1];
	strcpy(name, tempName);
}

CPlayer::~CPlayer(void)
{
	if (name != NULL)
	{
		delete [] name;
	}
}

CPlayer CPlayer::operator + (CPlayer & otherPlayer) //that operator being defined...

{
	CPlayer tempPlayer;
	tempPlayer.level = level + otherPlayer.GetLevel();
	return tempPlayer;
}

char * CPlayer::GetName()
{
	return name;
}

void CPlayer::LevelUp()
{
	level += 1;
	std::cout << "Duh do duh doo doo doo! Level up!" << std::endl;
}

int CPlayer::GetLevel()
{
	return level;
}

void CPlayer::Fight()
{
	std::cout << "Using a strength of " << strength << " ";
	std::cout << name << " kills the monster!" << std::endl;
}

int main()
{
	CPlayer player1(1,5);
	std::cout << "Your character is named: " << player1.GetName() << "\n" << std::endl;
	
	std::cout << "Here comes a monster!\n" << std::endl;

	player1.Fight();
	std::cout << "Good job!" << std::endl;
	player1.LevelUp();
	std::cout << "Now your level is: " << player1.GetLevel() << "!\n" << std::endl;

	std::cout << "See ya later!" << std::endl;

	CPlayer player2(player1);
	std::cout << "Welcome player2: " << player2.GetName() << std::endl;
	std::cout << "Player 2 level: " << player2.GetLevel() << std::endl;
	player2 = player2 + player2; //programs use of the operator

	std::cout << "Player 2 level now: " << player2.GetLevel() << std::endl;

	return 0;
}

Hope someone can see something wrong with this, I followed my book to a tee. I changed the operator function prototype and definition to return a type "CPlayer &" instead of just "CPlayer" and then changed the operator definition code to:

  CPlayer & CPlayer::operator + (CPlayer & otherPlayer)
{

	level = level + otherPlayer.GetLevel();
	return *this;
}

It now adds the levels correctly as it should. Why doesn't the first way work though, if anyone knows this, I would appreciate hearing, so I can know not to make that mistake. - GamerAlan [edited by - GamerAlan on February 22, 2003 2:18:58 AM]

Zipster

Feb 22, 2003 at 1:34 AM

Oh boy, you've got something going there. The basic reason why it's crashing is simply because you don't initialize name to 0 in the constructor. While this will fix that crash, another will appear again... if that's all you fix.

Let's examine this operator:


CPlayer CPlayer::operator + (CPlayer & otherPlayer) //that operator being defined...
{
     CPlayer tempPlayer;
     tempPlayer.level = level + otherPlayer.GetLevel();
     return tempPlayer;
}

When the temporary object is created, its name variable is some default values, 0xCCCCCCCC in a Debug build. When you return the object, a copy is created, however the original is destroyed.

Two things. Your destructor deletes the name if it is non-NULL, however if name is never initialized it will be non-NULL in a Debug build and the application will crash trying to delete the bogus pointer.

Second. Let's say name is 0. When the object is returned, it is copied. Let's take a look at your copy constructor:


CPlayer::CPlayer(CPlayer & copyPlayer)
{
     level = copyPlayer.level;
     strength = copyPlayer.strength;
     //char tempName[1024] = copyPlayer.GetName();
     char tempName[1024];
     strcpy(tempName, copyPlayer.GetName());
     int nameLength = strlen(tempName);
     name = new char[nameLength + 1];
     strcpy(name, tempName);
}

The copyPlayer parameter will reference the temporary, which we've determined to have a 0 name. The first call to strcpy will crash now because you're trying to copy from bad memory. A NULL check before you try to copy the name in this constructor will fix that problem.

To sum it up, zero out your name pointer in both constructors, and add a NULL check before you copy memory in the copy constructor. While I don't agree with your usage of that class as a temporary, given its design, those fixes will solve the crashes. However, the player2 = player2 + player2 line looks a little suspicious, it might cause problems, but I'm not sure.

[edited by - Zipster on February 22, 2003 2:38:26 AM]

GamerAlan

Feb 22, 2003 at 2:47 AM

Hey, good post there, Zipster. I see now why it would cause problems. My next question, since the book neglected to mention this. When I return a class object, such as returning an object of CPlayer, it calls my copy function? What then, if I have no copy function? I had originally only created the copy function to practice making copies through its use, and because I had planned to make a template of sorts for creating new players with using it(Not for any real game, but for my own practice).

I can see how bypassing the creation of a temporary object works then, since it never runs into the bad memory of name. Thanks for your help!

-
GamerAlan

civguy

Feb 22, 2003 at 3:03 AM

quote:
Original post by GamerAlan
What then, if I have no copy function?

You mean copy constructor? Well, if you haven''t defined one, a default copy constructor will be created by the compiler. It uses copy constructor for all of your class'' members (thus failing to copy dynamically allocated memory for example).

Fruny

Feb 22, 2003 at 3:12 AM

Watch out, you have implemented the binary operator+ with the semantics of operator+=. Writing a+b shouldn't modify a, but only return the sum of a and b.

Correct implementation would be, as a member function :

    
CPlayer CPlayer::operator+( const CPlayer& rhs ) // return by value !

{
  CPlayer tmp( *this );    // Make a copy

  tmp.level += rhs.level;  // No need to use a member function

  return tmp;              // return the modified copy

}

Or, as a non-member friend function (preferred)

  
CPlayer operator+( CPlayer lhs, const CPlayer& rhs ) 
// note: one parameter is passed by value.

// note: return by value !

{
  lhs.level += rhs.level;  // modify the local copy

  return lhs;              // return the modified copy

}

Or, assuming there is an CPlayer::operator+= implemented (best)

        
CPlayer operator+( CPlayer lhs, const CPlayer& rhs )
{
  lhs += rhs;
  return lhs;
}

Operator += would, of course be implemented as a member function

        
CPlayer& CPlayer::operator+=( const CPlayer& rhs ) // return by reference !

{
  level += rhs.level;  // No need to use a member function

  return *this;
}

GamerAlan

Feb 22, 2003 at 3:22 AM

Ahh I see. That probably explains why if I try to do something like player2 = player1 + player2; a whole new error comes up right at the time I call player2.GetName(), while player2 = player2 + player2 works fine. That or its a problem with my operator coding.

Anyone know of some good resources for creating class operators and copy constructors, since GPAIO seems to neglect that subject just a bit?

On a side note, Zipster, when you say zero name out, what exactly do you mean?

-
GamerAlan

Zipster

Feb 22, 2003 at 3:44 AM

Assign 0 to it:

name = 0;

Viola!

If you have a block of memory you want to zero out, you can use memset:

memset(myMemory, 0, nSizeOfMemory);

Or, if you''re the trendy C++ type, you can use the fill algorithm:

std::fill(myMemory, myMemory + nSizeOfMemory, 0);

Fruny

Feb 22, 2003 at 4:24 AM

quote:
Original post by GamerAlan
Anyone know of some good resources for creating class operators and copy constructors, since GPAIO seems to neglect that subject just a bit?

A. Koenig & B. Moo "Accelerated C++", ed. Addison-Wesley.
B. Stroustrup, "The C++ Programming Language", ed. Addison-Wesley.

GamerAlan

Feb 22, 2003 at 1:26 PM

Hey, I''m getting the operators to work a bit better, but the operator that accepts two parameters is throwing me off, its giving me a compile error, saying that the operator does not take two parameters. I wrote a new program that looks this:

  class CVector
{
private:
	int x;
	int y;
public:
	CVector();
	CVector(int theX, int theY);
	CVector(CVector & otherVector);
	~CVector();
	CVector operator + (CVector lhs, const CVector & rhs);
	
	int GetX();
	int GetY();
	void SetXY(int theX, int theY);

};

CVector::CVector()
{
	x = 0;
	y = 0;
}

CVector::CVector(int theX, int theY)
{
	x = theX;
	y = theY;
}

CVector::CVector(CVector & otherVector)
{
	x = otherVector.x;
	y = otherVector.y;
}

CVector::~CVector()
{

}

CVector CVector::operator + (CVector lhs, const CVector & rhs)
{
	lhs.x += rhs.x;
	lhs.y += rhs.y;
	return lhs;
}

int CVector::GetX()
{
	return x;
}

int CVector::GetY()
{
	return y;
}

void CVector::SetXY(int theX, int theY)
{
	x = theX;
	y = theY;
}

int main()
{
	CVector vecOne(1,1);
	CVector vecTwo(2,2);
	CVector vecThree(3,3);
	std::cout << vecOne.GetX() << ", " << vecOne.GetY() << std::endl;
	std::cout << vecTwo.GetX() << ", " << vecTwo.GetY() << std::endl;
	std::cout << vecThree.GetX() << ", " << vecThree.GetY() << std::endl;
	vecOne = vecTwo + vecThree;
	std::cout << vecOne.GetX() << ", " << vecOne.GetY() << std::endl;



	return 0;
}

-
GamerAlan

Fruny

Feb 22, 2003 at 4:33 PM

Read my examples again. If you define the binary operator to take two parameters, you must make it a non-member friend function.

dalleboy

Feb 23, 2003 at 6:11 AM

The keyword is friend (continuing Frunys example):

  
class CVector
{
.
.
.
   // Friends, we'll be friends. (Let the following function have access to all members)

   friend CVector operator+(CVector lhs, const CVector& rhs);
.
.
.
};
// Note that this is a non-member function:

CPlayer operator+(CPlayer lhs, const CPlayer& rhs)
{
   lhs += rhs;
   return lhs;
}

Update GameDev.net system time campaign - success at last

[edited by - dalleboy on February 23, 2003 7:12:59 AM]

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.