6

The constructor should, to my knowledge, be defined in the implementation file but I've only been able to find examples with the class inside one main file instead of split into a .h and .cpp file

All I need to know is if my following code is separated in an acceptable manner..

Entity.h:

    using namespace std;

class cEntity {
private:
    /*-----------------------------
    ----------Init Methods---------
    -----------------------------*/
    int *X, *Y;
    int *Height, *Width;

public:
    /*-----------------------------
    ----------Constructor----------
    -----------------------------*/
    cEntity (int,int, int, int);

    /*-----------------------------
    ----------Destructor-----------
    -----------------------------*/
    ~cEntity ();

    /*-----------------------------
    ----------Set Methods----------
    -----------------------------*/

    /*Set X,Y Methods*/
    void setX(int x){*X=x;};
    void setY(int y){*Y=y;};
    void setXY(int x, int y){*X=x; *Y=y;};

    /*Set Height, Width Methods*/
    void setHeight(int x){*Height=x;};
    void setWidth(int x){*Width=x;};
    void setDimensions(int x, int y){*Height=x; *Width=y;};

    /*-----------------------------
    ----------Get Methods----------
    -----------------------------*/

    /*Get X,Y Methods*/
    int getX(){return *X;};
    int getY(){return *Y;};

    /*Get Height, Width Methods*/
    int getHeight(){return *Height;};
    int getWidth(){return *Width;};
};

and Entity.cpp:

#include "Entity.h"


cEntity::cEntity (int x, int y, int height, int width) {
   X,Y,Height,Width = new int;
  *X = x;
  *Y = y;
  *Height = height;
  *Width = width;
}

cEntity::~cEntity () {
  delete X, Y, Height, Width;
}

I would also like to say thanks to everyone for being so helpful, especially on my first question!

Improve this question
9
  • I suggest looking into boost.shared_ptr / boost.scoped_ptr. They handle deletion on destruction and your risk of memory leaks is lower. Also, make the destructor virtual unless you have a reason for not doing so. Commented Nov 11, 2011 at 22:20
  • 2
    I suggest looking into non-pointer types. Commented Nov 11, 2011 at 22:21
  • 4
    delete X, Y, Height, Width; this does not do what you think it does. Commented Nov 11, 2011 at 22:22
  • also only Width will be allocated... Commented Nov 11, 2011 at 22:24
  • Alright I assume you mean I'd need a separate 'delete' per variable? And I'll read up on the boost library and virtual keyword. Commented Nov 11, 2011 at 22:25

5 Answers 5

Reset to default
5 
cEntity::cEntity (int x, int y, int height, int width) {

is correct

   X,Y,Height,Width = new int;

not so much. That sets Width to a new int, but not the rest. You probably intended:

   X = new int(x);
   Y = new int(y);
   Height = new int(height);
   Width = new int(width);

Note that this method of construction will not work for objects without assignment/copy, like references. For some objects, it's also slower than constructing them in place. As such, the preferred way to construct is like so: 

cEntity::cEntity (int x, int y, int height, int width) {
    :X(new int(x))
    ,Y(new int(y))
    ,Height(new int(height))
    ,Width(new int(width))
{}

This is better, but if any exceptions are thrown, you'll have to somehow deallocate the ones that were allocated. Better is to make each of those members a std::unique_ptr<int>, so they'll deallocate themselves and save you many headaches.

Improve this answer
Sign up to request clarification or add additional context in comments.

9 Comments

Interesting, thanks for showing me alternative construction methods.. I'm very new to all of this but I'm attempting to learn :P
Or don't make them pointers at all. Then their allocation is within the object itself and they will be freed when the cEntity object is destroyed.
@MichaelPrice: I assumed they were pointers for a reason, but you raise a good point.
I've removed all pointers from this example. However doesn't this imply that I will be unable to create new instances of my class in real time while the program is running?
@NickTgodSavage No. You can still say new cEntity(); or whatever derived class you want on the heap. Technically, even objects that live on the stack are created "in real time" (with the possible exception of non-reference, const objects [see constexpr in C++11] or with globals [allocated at program startup]).
|
3

Yes, it's OK. However, there is a problem with your constructor and destructor. What your code actually does is allocating one int and your destructor deallocates one int also. Anyway, there is no need to use pointers here. Somewhat better implementation (if we don't use smart pointers), could be:

[Entity.h]

private:
    /*Private fields*/
    int X, Y;
    int Height, Width;

[Entity.cpp]

cEntity::cEntity (int x, int y, int height, int width) {
  X = x;
  Y = y;
  Height = height;
  Width = width;
}
cEntity::~cEntity () {
}

And one more thing. Try to avoid using namespace std; in your header files. If you do, you force those who include your header to use this using statement and it can provoke namespace clashes.

Improve this answer

4 Comments

So would it be best to just make use of 'std::' or is there another way of excluding the namespace?
Nick, please see this question: Why shouldn't I put "using namespace std" in a header?
You don't need accessing std namespace in your code here. And yes, when you need, you'll use std::.
The std namespace IS needed further on in the code this is simply a section that I wanted information on. Further down I utilize strings and vectors as well.
1

Your separation is fine. The implementations of those functions is wrong, but you've separated them from the declaration suitably. (They don't allocate or free as many objects as you think they do.)

Improve this answer

Comments

0

Yes. For the separation at least, that's generally the best way to do it.

As for the actual implementation you have some issues. I am not really sure what you are trying to do with the constructor or if you have the correct data types for the class member variables but something seems off.

Improve this answer

Comments

-1

Any method defined in the class directly is implicitly inlined, including the constructor.

I.e. 

class MyClass  
{  
public:    
    MyClass() {};  
};

defines an inline constructor, which may (or may not) improve your code performance,

Whereas

class MyClass  
{  
public:  
    MyClass();  
};  

MyClass::MyClass()
{
};

is not inlined, and therefore won't have those benefits. Both options are correct C++ though.

Just my 2 cents.

P.S And yes, when you decide to store pointers inside a class in this manner, you open a Pandora box.

Improve this answer

3 Comments

Would it be more wise to just skip the implementation file for this scenario and use an inline constructor?
Depends. Interfaces are meant to be short, clean and easy to understand. If there's a lot of code in your constructor, it will pollute your interface file. Your 4-line constructor is probably OK inside .h though.
"Any method defined in the class directly is implicitly inlined" really doesn't make sense. The compiler will decide if something is going to be inlined based on some implementation defined behavior.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.