Freeing memory with delete vs smart pointer and proper way of freeing memory

Question

I'm doing a project for the university and im trying to find out how to delete memory properly and if the way of deleting that i came up with will have the same effect as using smart pointer.

This is a class that will hold all employees working in the company and teams which basically have vector of pointers to some of the employees.

class Company
{
private:
    std::string companyInfo;
    std::vector<Employee * > employees;
    std::vector<Team *> teams;
public:
    Company();
    ~Company();

    std::string getCompanyInfo() const;
    void setCompanyInfo(const std::string & companyInfo);
    bool addEmployee(Employee * employee, const std::string & teamName);
    bool addTeam(Team * team);
    void printTeams() const;
    void printEmployees() const;
};

So my question is : Is this a proper way of freeing the memory and if its is will it provide the same result as using smart pointers to automate the process.

Company::~Company()
{
    for (Employee * e : employees) {
        delete e;
    }

    employees.clear();

    for (Team * t : teams) {
        delete t;
    }

    teams.clear();
}

If it's better practise to use smart pointer should i use unique or shared pointer in my particular case. I have red about smart pointers and that they delete the allocated memory when the scope end but i'm really confused when will end. Is this going to happen when the destructor of company is called?

Thanks in advance and sorry if my questions look stupid.

Answer 1

You should never write delete in your code logic if you don't know what you're doing. That's the rule. Whenever you break this rule, expect all kinds of problems to happen. In other words: Always use std containers + smart pointers.

Unique pointer: A simple wrapper around your pointer that will delete the resource under it when it goes out of scope. You can't copy unique pointers, because then who will hold the ownership of the pointer if you can? Who will delete it? Only 1 object should be able to delete it.

Shared pointer: Makes copying possible, but at the expense of having an (atomic) counter that counts how many objects refer to the same pointer. When the counter goes to zero, the resource under the pointer is deleted.

Answer 2

Is this a proper way of freeing the memory

Assuming that the containers are filled with the new operator like this,

employees.push_back(new Employee());
teams.push_back(new Team());

then yes, this is the correct way to clean up these resources.

... and if its is will it provide the same result as using smart pointers to automate the process.

Generally yes. You can tweak how a smart pointer does its job of deleting the resource it manages, but the default behavior is the correct one for your case (call delete).

should i use unique or shared pointer

The first choice should be std::unique_ptr. It's more efficient than std::shared_ptr and is more restrictive on how the object it manages can be used. But this comes with limitations, e.g. a class with std::unique_ptr data members (like Company in your case) can't be copied. Using std::shared_ptr instead mitigates this issue, but comes with very different semantics - copying a Company object then means sharing teams and employees. This might not be what you want. One way around this issue is to go with std::unique_ptr, implement copy and copy assignment constructors for Company in terms of so-called virtual constructors Employee::clone() and Team::clone() member functions.

they delete the allocated memory when the scope end but i'm really confused when will end

An example:

void someFunction()
{
    Company c;

    // stuff...

} // the scope ends here, ~Company will be called

Answer 3

Is this a proper way of freeing the memory

The destructor is OK, assuming all of the stored pointers have been allocated using a new-expression, and assuming that all of them are still valid i.e. not dangling.

Clearing the vectors is redundant though. The vectors are about the be destroyed immediately, so their elements are going to be cleared anyway.

However, your class is most likely broken. It is copyable, and if you make a copy, and don't remove the pointers from one of the copies, then the destructors of those objects will delete the same pointers which will result in undefined behaviour.

You can fix this problem, as well as make your destructor simpler (you can simply use the implicit destructor) by using smart pointers.

should i use unique or shared pointer

Yes, you should.

Your code demonstrates unique ownership - Company owns the Employees and Teams uniquely - so the simplest change is to use unique pointers.

I have red about smart pointers and that they delete the allocated memory when the scope end but i'm really confused when will end. Is this going to happen when the destructor of company is called?

It is the destructor of the smart pointer that deletes the pointer.

When the smart pointer is a local variable, it is destroyed at the end of the scope. Just like if your Company is a local variable, it is destroyed at the end of scope, and its destructor will be run.

When the smart pointer is a member, it is destroyed when the super object is destroyed. In your case, you'd have a vector of smart pointers, in which case the smart pointer is destroyed when the vector is destroyed, or the smart pointer is erased from the vector.