Vector of class with pointer to sf::Sprite

[replicant]

I want to have a vector of a class that has a pointer to sf::Sprite as a class member, can anyone tell me if this would be correct? particularly the copy constructor and assignment operator (ignoring exception safety).

Code: [Select]

#ifndef PACCAT_PILL
#define PACCAT_PILL

#include "Entity.h"

#include <SFML/Graphics.hpp>

#include "Load.h"

class Pill : public Entity {
public:
static bool InitializeTexture(sf::RenderWindow& renderWindow)
{
return Load::LoadTextureFromFile(renderWindow, texture, "resources\\image\\pill.png");
}

Pill();
~Pill();
Pill(const Pill& other);
Pill& operator=(const Pill& rhs);

private:
static sf::Texture texture;

sf::Sprite* sprite;
};

#endif

Code: [Select]

#include "Pill.h"

sf::Texture Pill::texture;

Pill::Pill()
{
sprite = new sf::Sprite(texture);
}

Pill::~Pill()
{
delete sprite;
sprite = NULL;
}

Pill::Pill(const Pill& other)
{
sprite = new sf::Sprite(*other.sprite);
sprite->SetTexture(texture);
}

Pill& Pill::operator=(const Pill& rhs)
{
if (this != &rhs)
{
sprite = new sf::Sprite(*rhs.sprite);
sprite->SetTexture(texture);
Entity::operator=(rhs);
}
return *this;
}

[Laurent]

It looks good, but :
1 - why are you using a pointer to a sprite??? this is definitely not necessary, it adds complexity and potential problems for no gain
2 - your assignment operator should be implemented using copy & swap
3 - your assignment operator leaks (you never delete the old m_sprite), but using either 1- or 2- will solve this problem
4 - setting pointers to NULL in a destructor is useless, the object is destroyed and won't be used again

[replicant]

It looks good, but :
1 - why are you using a pointer to a sprite??? this is definitely not necessary, it adds complexity and potential problems for no gain
2 - your assignment operator should be implemented using copy & swap
3 - your assignment operator leaks (you never delete the old m_sprite), but using either 1- or 2- will solve this problem
4 - setting pointers to NULL in a destructor is useless, the object is destroyed and won't be used again

Thanks for the fast reply. Now that you mention it, I don't actually need to use a pointer for this class...  :oops:

This would be correct for non pointer sprite?

Code: [Select]

#include "Pill.h"

sf::Texture Pill::texture;

Pill::Pill()
{
}

Pill::~Pill()
{
}

Pill::Pill(const Pill& other)
{
sprite = other.sprite;
sprite.SetTexture(texture);
}

Pill& Pill::operator=(const Pill& rhs)
{
if (this != &rhs)
{
sprite = rhs.sprite;
sprite.SetTexture(texture);
Entity::operator=(rhs);
}
return *this;
}

[Laurent]

Yes.

Now, if you want to improve your code, you can have a look at the following things:
- initializer list (to use in the copy constructor)
- copy & swap implementation of the assignment operator

[replicant]

Yes.

Now, if you want to improve your code, you can have a look at the following things:
- initializer list (to use in the copy constructor)
- copy & swap implementation of the assignment operator

Thanks! will check them out now.