views:

84

answers:

3

Please look at the following code and tell me if it's going to cause problems in the future, and if yes, how to avoid them.

class Note
{
   int id;
   std::string text;

public:
   // ... some ctors here...

   Note(const Note& other) : id(other.id), text(other.text) {}

   void operator=(const Note& other) // returns void: no chaining wanted
   {
      if (&other == this) return;
      text = other.text;  
      // NB: id stays the same!    
   }
   ...
};

In short, I want the copy constructor to create an exact copy of an object, including its (database) ID field. On the other hand, when I assign, I want just to copy the data fields. But I have some concerns, since usually the copy ctor and the operator= have same semantics.

The id field is used only by Note and its friends. For all other clients, the assignment operator does create an exact copy. The use case: When I want to edit a note, I create a copy using copy ctor, edit it, and then call save on the Notebook class which manages Notes:

 Note n(notebook.getNote(id));
 n = editNote(n); // pass by const ref (for the case edit is canceled)
 notebook.saveNote(n);

When on the other hand I want to create a completely new note with the same contents as an existing note, I can do this:

 Note n; 
 n = notebook.getNote(id); 
 n.setText("This is a copy");
 notebook.addNote(n);

Is this approach plausible? If not, please point out what the possible negative consequences are! Thanks a lot!

+7  A: 

If you want semantics that don't match what's expected from the assignment operator, then don't use it. Instead, disable it by declaring a private operator= and define a function with a name that makes clear what's going on, like copyDataFields.

aem
Is it ok to have a public copy ctor but private op=?
Alex Jenter
@Alex: Yes. It means once you have a fully initialized `Note`, it cannot be assigned. If it wants data, it would use the `copyDataFields` method. It can still be constructed to match another note: `Note othernote(firstNote);`
GMan
+2  A: 

Although this might work for your specific case, I would not recommend it in general.

Libraries such as the STL expect the copy constructor and assignment operator to work "like they're supposed to". If you violate the C++ semantics, then you may find that STL containers of your objects won't work right. The STL will invoke both your copy constructor and your assignment operator, under different circumstances, depending on the container.

It's very easy to get totally confused when your code doesn't do what you think it does.

d3jones
+1  A: 

Technically, it is doable and technically will work, but I would not do it that way. The problems I see are:

  1. You change "natural" semantic of assignment operator as known by C++ population.

  2. The two twin-operations, copy construction and assignment, are inconsistent because of different semantic.

  3. The solution is error prone because it's easy to accidentally call copy constructor even if it looks as assignment. If a programmer writes your second use case this way:

    Note n = notebook.getNote(id);
    

    Then copy constructor is called, not assignment, so you get n as different object than expected.

Why not to make your intentions just clear and explicit:

Note& Notebook::editNote(int id);
Note  Notebook::createNote(int id);
mloskot
I don't want to expose a Note for editing directly, I want a kind of "copy, edit, replace original" behavior.
Alex Jenter
I hardly see what's the difference between "copy, edit, replace original" and "get original, edit"
mloskot