All the best.
I am doing an OOP practice in C++. The theme (I don't understand why) is something from StarWars. The thing is that a ship (StarFighter) is made up of a series of parts (an attribute in an array of pointers to parts), as detailed in the header file:
#ifndef STARFIGHTER_H
#define STARFIGHTER_H
#include <string>
#include <stdexcept>
#include "Pieza.h"
/**
* @brief
*/
class StarFighter
{
private:
static const int MAX_PIEZAS = 50;
static int _numStarFighters; ///< Número de objetos de esta clase instanciados
int _idSF=0; ///< Identificador único de la nave
std::string _marca; ///< Marca de la nave (parece que las VW contaminan más)
std::string _modelo; ///< Modelo de la nava
int _numPlazas = 1; ///< Número de plazas de la nave
int _numPiezas = 1; ///< Numero de piezas de la nave
Pieza *_piezas[MAX_PIEZAS]; ///<Array de piezas de la nave
public:
StarFighter ();
StarFighter ( std::string marca, std::string modelo, int numpiezas );
StarFighter ( const StarFighter& orig );
virtual ~StarFighter ( );
StarFighter& setNumPlazas ( int numPlazas );
int getNumPlazas ( ) const;
StarFighter& setModelo ( std::string modelo );
std::string getModelo ( ) const;
StarFighter& setMarca ( std::string marca );
std::string getMarca ( ) const;
int getIdSF ( ) const;
std::string toCSV () const;
StarFighter& operator= ( const StarFighter& otro );
void fromCSV ( std::string& datos );
void addPieza(std::string nombre, float peso, std::string descripcion);
void quitarPieza(int posicion);
};
#endif /* STARFIGHTER_H */
The problem comes in the destruction of the ship. Being a composition, it is the ship that must be in charge of destroying the piece-type objects, but I don't know how I have to implement the destroyer for that to happen. I have it right now like this:
StarFighter::~StarFighter ( )
{
for (int i = 0; i < MAX_PIEZAS; i++){
this->_piezas[i] = nullptr;
}
}
But that never calls the destructor of the Part class (I've checked with the debugger ), so I understand that memory leak will occur. I have tried to change the constructor and put it as follows:
StarFighter::~StarFighter ( )
{
for (int i = 0; i < MAX_PIEZAS; i++){
delete [] this->_piezas;
}
}
Which brings me to my now familiar "Segmentation Fault" (I don't understand why either, but hey).
Before finishing, in case it has been left up in the air, I clarify that the constructor creates pieces up to the number indicated (by default 1):
StarFighter::StarFighter ( string marca, string modelo, int numpiezas ):
_marca (marca), _modelo(modelo), _numPiezas(numpiezas)
{
_numStarFighters++;
_idSF = _numStarFighters;
for (int i = 0; i < MAX_PIEZAS; i++){
_piezas[i] = nullptr; //Inicializamos todo el vector a nullptr
}
for (int i = 0; i < _numPiezas; i++){
_piezas[i] = new Pieza;
}
}
I appreciate any suggestion. This subject has homework, and being without classes doesn't help.
Thank you very much.
Let's see your attempts:
With that, as you say, you do not free memory. Your suspicions are true. You assign a new value to each pointer, but the memory it pointed to is never freed.
Well, now you have it. That code attempts to free the entire contents of the array , but this is incorrect as not all elements are valid pointers. Attempting to free a block of memory that has not been previously allocated is undefined behavior . And a possible consequence of that is what you have already observed:
Segmentation Fault
.The correct way would be... well, simply do the same as in the constructor, but instead of calling , well
new
, callingdelete
:-)That depends on how they are constructed, seeing as you store a collection of pointers:
What do you fill with
new
:You should delete each one with
delete
:So far the question, the best that can be answered with the few data you give. Now a review of the rest of the incorrect or improvable things in your code:
This question talks about it. Your member variables don't satisfy either of the two points, but it's best to avoid those names in case they conflict with other names.
The use of raw pointers is discouraged in modern C++, the use of arrays is also considered impractical. You should change the formation to one
std::array
formed by smart pointersstd::unique_ptr
:I have removed the label
private
at the beginning of the class because that is the default visibility of type objectsclass
, I have also created an alias of the type single pointer toPieza
that facilitates its use within the object and finally I have indicated that the constantMAX_PIEZAS
is a constant expression, which denotes its intent in addition to allowing the compiler to make optimizations.To add items
Pieza
to your collection, just create a new item in the correct position:And you don't have to worry about the destructor, because smart pointers are already automatically destroyed when their lifecycle ends.