Is this the easiest way to backup objects?

Discussion in 'C++' started by k04jg02@gmail.com, Aug 12, 2006.

  1. Guest

    Problem:

    I have a properties dialog. X objects build the dialog, but a subclass
    of X, such as Y, can add more options to the dialog for Y specific
    properties. I would like to write code for the dialog that creates a
    backup of the current object when the dialog pops up, and sets the
    current modified object to the backup when the user clicks cancel (or
    discards the backup if the user clicks OK). X already has a Clone
    method.

    (I realize this way of making backups isn't always practical for large
    objects, but the ones I'm dealing with are fairly lightweight)

    Solutions:

    0. Simply write the code using X pointers. Problem: Then only the X
    members get set because the assignment operator can't be virtual.

    1. Template the dialog class so you can do Dialog<X> or Dialog<Y>,
    causing a backup of the appropriate type to be made. Problem: The
    dialog code gets a lot uglier just to make 1 line work.

    2. Solution below seemed best, but I'm looking for feedback.

    This is a compileable test case.

    #include <iostream>

    template<class T>
    class CanBackup {
    public:
    CanBackup();
    ~CanBackup();
    void MakeBackup();
    void RestoreBackup();

    virtual CanBackup<T>* Clone() = 0;

    private:
    T* backup;
    };

    template<class T>
    CanBackup<T>::CanBackup()
    : backup(NULL)
    {
    }

    template<class T>
    CanBackup<T>::~CanBackup()
    {
    if(backup)
    delete backup;
    }

    template<class T>
    void CanBackup<T>::MakeBackup()
    {
    backup = (T*)Clone();
    }

    template<class T>
    void CanBackup<T>::RestoreBackup()
    {
    *((T*)this) = *backup;
    }

    class A : public CanBackup<A> {
    public:
    A();
    ~A();
    A* Clone();
    int x;
    };

    A::A() {}
    A::~A() {}

    A* A::Clone()
    {
    return new A(*this);
    }

    // Should print: 3 5 3
    // Actually prints: 3 5 5
    int main() {
    A foo;
    foo.x = 3;
    std::cout << foo.x << std::endl;
    foo.MakeBackup();
    foo.x = 5;
    std::cout << foo.x << std::endl;
    foo.RestoreBackup();
    std::cout << foo.x << std::endl;
    }
    , Aug 12, 2006
    #1
    1. Advertising

  2. wrote:
    > Problem:
    >
    > I have a properties dialog. X objects build the dialog, but a subclass
    > of X, such as Y, can add more options to the dialog for Y specific
    > properties. I would like to write code for the dialog that creates a
    > backup of the current object when the dialog pops up, and sets the
    > current modified object to the backup when the user clicks cancel (or
    > discards the backup if the user clicks OK). X already has a Clone
    > method.
    >
    > (I realize this way of making backups isn't always practical for large
    > objects, but the ones I'm dealing with are fairly lightweight)
    >
    > Solutions:
    >
    > 0. Simply write the code using X pointers. Problem: Then only the X
    > members get set because the assignment operator can't be virtual.


    Says who? Have you tried making it virtual? Did it not work?

    > 1. Template the dialog class so you can do Dialog<X> or Dialog<Y>,
    > causing a backup of the appropriate type to be made. Problem: The
    > dialog code gets a lot uglier just to make 1 line work.
    >
    > 2. Solution below seemed best, but I'm looking for feedback.
    >
    > This is a compileable test case.
    >
    > #include <iostream>
    >
    > template<class T>
    > class CanBackup {
    > public:
    > CanBackup();
    > ~CanBackup();
    > void MakeBackup();
    > void RestoreBackup();
    >
    > virtual CanBackup<T>* Clone() = 0;


    Please drop the "<T>" inside the template. It only confuses
    the readers of your code and adds nothing to it.

    >
    > private:
    > T* backup;
    > };
    >
    > template<class T>
    > CanBackup<T>::CanBackup()
    > : backup(NULL)
    > {
    > }
    >
    > template<class T>
    > CanBackup<T>::~CanBackup()
    > {
    > if(backup)
    > delete backup;


    There is no need to test 'backup' before deleting. "delete NULL;" is
    a NOP.

    > }
    >
    > template<class T>
    > void CanBackup<T>::MakeBackup()
    > {
    > backup = (T*)Clone();


    Why do you need to cast? I would rather expect either 'Clone' to
    return a T* or 'backup' to be of type 'CanBackup*'. See below.

    > }
    >
    > template<class T>
    > void CanBackup<T>::RestoreBackup()
    > {
    > *((T*)this) = *backup;


    This is utterly dangerous. Who told you that it's even going
    to work? Casting 'this' like that is an invitation for a big
    trouble.

    If you make 'backup' to be of type 'CanBackup*', then you can
    use 'dynamic_cast<T*>' here and only assign if you get non-null
    ponter back...

    > }
    >
    > class A : public CanBackup<A> {
    > public:
    > A();
    > ~A();


    You don't need those.

    > A* Clone();

    ^^^^^^^^^^^
    This should probably be private.

    > int x;
    > };
    >
    > A::A() {}
    > A::~A() {}
    >
    > A* A::Clone()
    > {
    > return new A(*this);
    > }
    >
    > // Should print: 3 5 3
    > // Actually prints: 3 5 5


    Of course. You're using some very dangerous constructs.

    > int main() {
    > A foo;
    > foo.x = 3;
    > std::cout << foo.x << std::endl;
    > foo.MakeBackup();
    > foo.x = 5;
    > std::cout << foo.x << std::endl;
    > foo.RestoreBackup();
    > std::cout << foo.x << std::endl;
    > }


    So, I'd rewrite it as

    template<class T>
    class CanBackup {
    public:
    CanBackup() : backup(NULL) {}
    virtual ~CanBackup() { delete backup; }
    void MakeBackup();
    void RestoreBackup();

    virtual CanBackup* Clone() = 0;

    private:
    CanBackup* backup;
    };

    template<class T>
    void CanBackup<T>::MakeBackup()
    {
    backup = Clone();
    }

    template<class T>
    void CanBackup<T>::RestoreBackup()
    {
    T* that = dynamic_cast<T*>(backup);
    T* self = dynamic_cast<T*>(this);
    if (that && self)
    *self = *that;
    }

    class A : public CanBackup<A> {
    public:
    CanBackup<A>* Clone();
    int x;
    };

    CanBackup<A>* A::Clone()
    {
    return new A(*this);
    }

    Now, for me this (along with your 'main') will print 3 5 3.
    There is still some protection you need to do, probably, to ensure
    that nobody does this:

    class A { ... };
    class B : public CanBackup<A> { ...

    V
    --
    Please remove capital 'A's when replying by e-mail
    I do not respond to top-posted replies, please don't ask
    Victor Bazarov, Aug 12, 2006
    #2
    1. Advertising

  3. mlimber Guest

    wrote:
    > Problem:
    >
    > I have a properties dialog. X objects build the dialog, but a subclass
    > of X, such as Y, can add more options to the dialog for Y specific
    > properties. I would like to write code for the dialog that creates a
    > backup of the current object when the dialog pops up, and sets the
    > current modified object to the backup when the user clicks cancel (or
    > discards the backup if the user clicks OK). X already has a Clone
    > method.
    >
    > (I realize this way of making backups isn't always practical for large
    > objects, but the ones I'm dealing with are fairly lightweight)

    [snip]

    Consider using standard serialization/unserialization techniques. See
    these FAQs:

    http://www.parashift.com/c -faq-lite/serialization.html

    and this handy library:

    http://boost.org/libs/serialization/doc/index.html

    Cheers! --M
    mlimber, Aug 12, 2006
    #3
  4. Guest

    Victor Bazarov wrote:
    > wrote:
    >


    > > 0. Simply write the code using X pointers. Problem: Then only the X
    > > members get set because the assignment operator can't be virtual.

    >
    > Says who? Have you tried making it virtual? Did it not work?


    Sorry, I should have explained more. The idea is that this can be used
    with the compiler generated assignment operator, which is not virtual.
    Now that I think about it the Clone method is probably not needed do to
    the template either...

    >
    > There is no need to test 'backup' before deleting. "delete NULL;" is
    > a NOP.


    I'd read so much stuff telling me not to free() NULL pointers I thought
    this carried over ;P Thanks :)

    >
    > > template<class T>
    > > void CanBackup<T>::MakeBackup()
    > > {
    > > backup = (T*)Clone();

    >
    > Why do you need to cast? I would rather expect either 'Clone' to
    > return a T* or 'backup' to be of type 'CanBackup*'. See below.


    Without it the code doesn't compile. You can't implicitly cast
    CanBackup<X*> to X*.

    >
    > > template<class T>
    > > void CanBackup<T>::RestoreBackup()
    > > {
    > > *((T*)this) = *backup;

    >
    > This is utterly dangerous. Who told you that it's even going
    > to work? Casting 'this' like that is an invitation for a big
    > trouble.
    >
    > If you make 'backup' to be of type 'CanBackup*', then you can
    > use 'dynamic_cast<T*>' here and only assign if you get non-null
    > ponter back...


    I've rewritten it to this:

    dynamic_cast<T&>(*this) = *backup;

    This way an exception gets thrown because you can't have a NULL
    reference.

    >
    > > A* Clone();

    > ^^^^^^^^^^^
    > This should probably be private.


    Good point :p

    > > // Should print: 3 5 3
    > > // Actually prints: 3 5 5

    >
    > Of course. You're using some very dangerous constructs.


    Whoops, it does actually print 3 5 3. I forgot to remove those old
    comments from before I had it working :)
    , Aug 12, 2006
    #4
  5. schrieb:
    > Victor Bazarov wrote:
    >> wrote:
    >>> template<class T>
    >>> void CanBackup<T>::MakeBackup()
    >>> {
    >>> backup = (T*)Clone();

    >> Why do you need to cast? I would rather expect either 'Clone' to
    >> return a T* or 'backup' to be of type 'CanBackup*'. See below.

    >
    > Without it the code doesn't compile. You can't implicitly cast
    > CanBackup<X*> to X*.


    You can't even _explicitly_ cast CanBackup<X*> to X* and get a sane result.
    And you cast a CanBackup<X*>* to X*, what makes it even worser.

    What makes you think that this should work?

    --
    Thomas
    Thomas J. Gritzan, Aug 12, 2006
    #5
  6. Ian Collins Guest

    wrote:
    >
    >>There is no need to test 'backup' before deleting. "delete NULL;" is
    >>a NOP.

    >
    >
    > I'd read so much stuff telling me not to free() NULL pointers I thought
    > this carried over ;P Thanks :)
    >

    Calling free() with a null pointer is safe.

    --
    Ian Collins.
    Ian Collins, Aug 12, 2006
    #6
  7. Guest

    Thomas J. Gritzan wrote:
    > schrieb:
    > > Victor Bazarov wrote:
    > >> wrote:
    > >>> template<class T>
    > >>> void CanBackup<T>::MakeBackup()
    > >>> {
    > >>> backup = (T*)Clone();
    > >> Why do you need to cast? I would rather expect either 'Clone' to
    > >> return a T* or 'backup' to be of type 'CanBackup*'. See below.

    > >
    > > Without it the code doesn't compile. You can't implicitly cast
    > > CanBackup<X*> to X*.

    >
    > You can't even _explicitly_ cast CanBackup<X*> to X* and get a sane result.
    > And you cast a CanBackup<X*>* to X*, what makes it even worser.
    >
    > What makes you think that this should work?


    Well, that it does work. I cast CanBackup<X>* to X*, not CanBackup<X*>*
    to X* (note only one asterisk in the first). In practice X should
    always be a subclass of CanBackup<X> (that's intended usage, I could
    probably add safeguards to enforce this).
    , Aug 13, 2006
    #7
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. tom
    Replies:
    4
    Views:
    14,076
    jpowers5882
    Oct 24, 2008
  2. Rolf Hemmerling
    Replies:
    7
    Views:
    366
    Corwin Joy
    Nov 15, 2003
  3. gavino
    Replies:
    13
    Views:
    1,028
  4. Simon Strandgaard
    Replies:
    3
    Views:
    93
    Simon Strandgaard
    Jul 22, 2005
  5. Replies:
    0
    Views:
    567
Loading...

Share This Page