No, it doesn't look quite right. A class constructor is called upon an instance of the class, so that instance has access to private members (variables and functions) defined in that class.

As such, you can write a copy constructor like this:

class Foo
{
private:
   string my_string;
   int my_int;

public:
   Foo(const Foo & other)
      : my_string(other.my_string) , my_int(other.my_int)
   {
   }
};

See how I accessed the other's privates directly? I didn't have to call accessor functions like getMyString() or getMyInt().


Well, the name of the function implies that you're returning a Publisher object. You need to be careful that the code calling getPublisher() doesn't mess up the memory allocated for the original object. If you return a Publisher object, odds are you're making a copy of the original object via the assignment operator. That is:

Publisher pub = someBookItem.GetPublisher();


To use the copy constructor, you'd do this:

Publisher pub(someBookItem.GetPublisher());


Either way creates a copy of the original so that when the copy goes out of scope, the original is still in tact.


one more thing -- your SetPublisher() function deletes the existing publisher pointer without checking if it's NULL. It might be best for you to implement an operator=() function to handle assignment. That way, you won't have to rely on managing pointers to objects, you can just deal with objects themselves.

The operator=() declaration would look a lot like your copy constructor. To use the Foo example class above:

class Foo
{
   ...

   Foo& operator=(const Foo & other)
   {
      my_string = other.my_string;
      my_int = other.my_int;
   }
};


Finally, notice that I don't use "this->" anywhere. Within the context of an object instance's member functions, "this->" is implicit when accessing that instance's member variables. Granted, one might argue that it makes the code more legible and obvious. However, I disagree: If you adopt a variable naming convention that identifies member variables, then you don't need "this->". that's 6 extra characters on each line of code that you don't need. This might not seem like a lot, but keep in mind that most institutions (companies, schools, etc) impose formatting rules for code (aka a "coding standard"). If you are not supposed to have lines extend past 80 characters, then it makes sense to be as consise as possible.

For example, many people use the "m_" prefix to member variables. Other people prefer a trailing underscore. Here's both in our Foo example:

// m_

class Foo
{
private:
   string m_my_string;
   int m_my_int;
};


// trailing underscore
class Foo
{
private:
   string my_string_;
   int my_int_;
};


It becomes immediately apparent, therefore, when you're acecssing a member variable, and "this->" becomes unnecessary. Local variables do not include these characteristics, so it's easy to distinguish the two.


Take care,

Nik
http://www.bigaction.org/