0
\$\begingroup\$

Every Account has its owner who is Person The bank has list of owners. I am not sure if my destructors are right and delete bank will delete all elements of list<Account*>* v nad the list itself. Please help.

#include <iostream>
#include <vector>
#include <list>
using namespace std;
class Person{
public:
    char* name;
    int age;

    Person(char* name, int age){
        this->name = name;
        this->age = age;
    }
    ~Person(){
    }

    void show(){
        cout<<name<<" "<<age<<" yo";
    }
};

class Account{
public:
    Person& owner;
    double money;
    Account(Person* owner, double money):
        owner(*owner) , // this->owner = *owner;
        money(money)  { //Uninitialized reference member
    }

     void show(){
        cout<<"\n-------------\n";
        owner.show();
        cout<<endl;
        cout<<money<<"USD\n-------------";
    }
      ~Account(){
    }
};

class Bank{
public:
    list<Account*>* v;
    Bank(){
        v = new list<Account*>();
    }
    Account* openNewAccount(Person* client, double money){
        Account* r = new Account(client, money);
        v->push_back(r);
        return r;
    }
    void zmienWlasciciela(Person& o, Account* r){

        r->owner = o;
    }
    void usunRachunek(Account* r){
        cout<<"\n\n\nUSUNALEM"<<endl;
       v->remove(r);
       delete r;
    }
    void show(){
    for (list<Account*>::iterator it = v->begin(); it != v->end(); ++it){
        (*it)->show();
    }
    }
    ~Bank(){
        delete v;
    }
};



int main(){
    Bank* bank = new Bank();
    Person* thomas = new Person("thomas", 34);
    Account* r1 =  bank->openNewAccount(thomas, 64363.32);
    Account* r2 =  bank->openNewAccount(thomas, 41251.54);
    Account* r3 =  bank->openNewAccount(thomas, 3232.32);
    bank->show();

    Person* margaret = new Person("Margaret", 23);
    bank->zmienWlasciciela(*margaret, r2);
    cout<<"I have changed owner of account r2"<<endl;
    bank->show();
    cout<<"I have deleted account r3"<<endl;
    bank->usunRachunek(r3);
    bank->show();

    delete bank;
    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ No, it's wrong. But why on earth are you using a pointer to a list? \$\endgroup\$
    – Yuushi
    May 19, 2013 at 13:02

2 Answers 2

5
\$\begingroup\$

General Comment on Style

This is probably not what you want.

Bank* bank = new Bank();
Person* thomas = new Person("thomas", 34);

You actually want:

Bank   bank;
Person thomas("thomas", 34);

This creates automatic variables. That are properly destroyed automatically at the end of scope.

It is very rare to use RAW pointers in C++ Bank* or Person*. When you dynamically allocate objects using new you want to make sure that the returned pointer is put into an object that controls their lifespan and garbage collects the values when you are finished using them.

std::shared_ptr<Bank> bank = new Bank;  // This is the closest to a Java object 
                                        // that C++ has. It allows multiple users
                                        // and it is correctly destroyed when nobody
                                        // has a reference to it anymore.

Looking at the code:

Don't do this:

using namespace std;

I know all the shitty C++ books you read have it in. Its fine for 10 line long programs you type out of a book. But once you get past those programs it can cause problems with namespace pollution. Its thus a very bad habit and one you should break as soon as possible. See: https://stackoverflow.com/q/1452721/14065

Person

Pointers are bad.

    char* name;

Unless you do a lot of validation and know the ownership of the thing you are pointing at and it is not going to be cleaned up behind you then you should be very weary of using pointers. I have checked your code and you are fine in this simple program but this is not a good design outside of a small toy project. C++ also has its own string type std::string so you don't need to know or use C-String. So I would change this to

    std::string  name;

In constructors prefer to use the initializer list to initialize the members.

    Person(char* name, int age){
        this->name = name;
        this->age = age;
    }

Second point about pointers here. Its not normal to pass pointers to around in C++ (this is very common in C but modern C++ you should basically never do it (with a few exceptions that you will work out when you have more experience)). Pass objects by reference if you want to retain ownership; otherwise pass a smart_pointer to transfer ownership of the object.

In this case you want to pass a std::string. As you don't want to change the original but make your own copy pass a const reference (if you are in C++11 you may want to also add a move constructor (but this is advanced so don't worry about it)).

So it should look more like:

    Person(std::string const& name, int age)
        : name(name)
        , age(age)
    {}

If a destructor does not do anything then don't declare it.

    ~Person(){
    }

Its fine to have a public show() method. But any method that does not change the state of the object should be declared as const. This allows you to call the object from const objects (useful technique later). And if you add a parameter so it can be any stream it makes it more versatile. But you should also declare a stream operator for your Person type. Thus you can stream your person just like any other type.

    void show(std::ostream& out = std::cout) const {
                                        //   ^^^^^    Does not change the state
                                        //            so make the method const.
          //  ^^^^^^^^^^^^^^^^^  Pass stream to print on.
          //                     You may want to print in other places.
          //                     rather than std::cout (like a file).

        out<<name<<" "<<age<<" yo";  // A few spaces to make it readable
                                      // would be nice.
    }

// Pass person by const reference so the compiler
// knows you are not going to modify the object.
// This means you can only call const methods (so `show()` must be const.
std::ostream& operator<<(std::stream& stream, Person const& person)
{
    person.show(stream);
    return stream;
}

int main()
{ std::cout << Perons("Loki", 962) << "\n";
}

Account

This is fine.

Person& owner;

It means you have a reference to a person that is stored somewhere else. But you need to be careful of lifetimes. Make sure the Person object is still alive while you are alive.

Passing by pointer again.

Account(Person* owner, double money):

This is dangerous. Especially since you de-reference without checking. Since you are storing a reference. Pass the value in by reference.

Account(Person& owner, double money)
    : owner(owner)
    , money(money)
{}

Make the show() method const and pass a stream.

 void show(std::ostream& out = std::cout) const {
                                      //  ^^^^^
    out<<"\n-------------\n" << owner << std::endl << money<<"USD\n-------------";
    return out;
}

Add a stream operator

std::ostream& operator<<(std::stream& stream, Account const& account)
{
    account.show(stream);
}

Destructor does nothing so remove it.

  ~Account(){
}

Bank

Ok. Obviously a java (or java inspired language user).

list<Account*>* v;

You don't want to use pointers here. By making this a pointer you need to do all sorts of memory management (and make sure you obey the rule of three (look it up)). But you don't need to do that just make it a normal object and all those problems go away.

list v;

When the bank object is created. It automatically creates a list now. When the bank is destroyed it automatically cleans up its members (including the list). When the back is copied it automatically copies the accounts to the new Bank.

Second; don't make the Account's inside the list pointers. Again you will need to manage the each object and control its lifespan. Much easier to put Account objects into the list.

list v;

There is also a principle called Separation of Concerns. Basically this says that an type should either be Business Logic or Resource Management you should not do both in the same type. Since Bank is obviously business logic you should not be doing resource management in the Bank. If you were using pointers then you would delegate the memory management of those pointers to anther type that would correctly do the clean up. C++ already has a whole bunch of smart pointers explicitly for managing pointers.

BUT in this situation I would defiantly sidestep the memory management by using automatic objects and make the changes I described above as ALL your memory management issues disappear.

Remember use initializer list.

Bank(){
    v = new list<Account*>();
}

But if you change the type as (described above) you don't need to do anything. The object automatically creates and initializes all members.

Again passing pointers is bad:

Account* openNewAccount(Person* client, double money){
    Account* r = new Account(client, money);
    v->push_back(r);
    return r;
}

If you return a pointer Account* you don't specify any ownership semantics. If you don't specify ownership semantics then you are very liable to leak as nobody knows who is supposed to clean up. In this situation the Account obviously belongs to the Bank object (The Bank will do the memory management). So here you should return a reference. This is an indication you can use the account. But the account will be managed by the Bank.

Account& openNewAccount(Person const& client, double money)
{
    Account  newAccount(client, money);  // Create a new account.
    v->push_back(newAccount);            // Not when you push objects into containers
                                         // they make a copy of the object.
                                         // So you have a copy of `newAccount`


    return v.last();   // return a reference to the last member of `v`
                       // that you just pushed there via `push_back`
                       // The user can now operate on the referenc
                       // Also the reference will never be NULL
                       // so the user needs to check for that.
}

Pass by reference. You don't need to check for NULL

void zmienWlasciciela(Person& o, Account* r)

// Becomes.
void zmienWlasciciela(Person& o, Account& r){
    r.owner = o;
}

Pass by reference (that's how it was given to you).

void usunRachunek(Account* r)

// Becomes
void usunRachunek(Account* r){
    cout<<"\n\n\nUSUNALEM"<<endl;
   v.remove(r);
}

Make show() const and take a stream

void show(std::ostream& out = std::cout ) const {
for (list<Account*>::iterator it = v.begin(); it != v.end(); ++it){
    (*it)->show(out);
}

You could use a simple algorithm to copy the Account to the stream.

void show(std::ostream& out = std::cout ) const
{
    // for this to work Account must have operator<< defined.
    std::copy(v.begin(), v.end(), std::ostream_iterator(out));
}

}

This is correct BUT NOT ENOUGH for your original definition where v is a pointer. If you use objects you don't need to do anything.

~Bank(){
    delete v;
}

If you were using pointers then you also need to define a copy constructor and assignment operator to make sure you are not violating the rule of three. If you are using objects the compiler generated defaults work perfectly fine.

\$\endgroup\$
0
\$\begingroup\$

Deleting the vector will only delete the pointers in the vector, not the things to which they point.

You need to iterate though the vector deleting the accounts before deleting the vector.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.