1

Given:

Class A
{
     private:
       double **CR;
     public:
       double **compute2D();
};

Suppose that I have a private 2D Array member:

double **CR;

And I have a member function:

Double ** Compute2D()

Function computer2D will return CR.

Is this a bad practice? Why? Should I use the getter and setter functions to return it?

And one more question: Am I using the garbage collector correctly?

A::~A()
{
    //Delete 2D array
    for(int i = 0; i < Rows; ++i)
    {
        delete [] CR[i];
    }

    delete [] CR;
}
Bob2Chiv
  • 1,858
  • 2
  • 19
  • 29
Hani Goc
  • 2,371
  • 5
  • 45
  • 89
  • 2
    "Am i using the garbage collector correctly?" - No, since there is no garbage collector. You appear to be deleting the allocated memory correctly, but if you want to mess around with `new` and `delete` then you'll need to implement or prevent copying per the ["Rule of Three"](http://stackoverflow.com/questions/4172722). Better still, use a RAII class like `std::vector` to manage the memory for you (per the "Rule of Zero"). – Mike Seymour Jul 29 '13 at 12:59
  • you might want to consider returning a const pointer to prevent other classes by changing the private member. This can be circumvented by using reinterpret_cast but that would generally also be considered bad style – Serve Laurijssen Jul 29 '13 at 13:02

6 Answers6

1

By doing this, you allow changing private member value, outside of the class. It is up to you if it is bad or good. But in my opinion, if I have a private member, then I must be the only one who can change its value.

Ali Burak Kulakli
  • 562
  • 1
  • 5
  • 16
  • yes I see @Ali thank you. I think well I must change it. But Even As I said to Quinxorin if I use a getter function a user can modify its values. – Hani Goc Jul 29 '13 at 13:01
1

It's bad to return a pointer to private member of your class because it can break encapsulation. Somebody will be able to change value of private variable without
inform your class about it. However there is nothing bad in return copy of fields of your class. When using getter and setter there is no way to change class state without class knowing about it. So that's nice practice.

With deleting an array everything looks ok.

Leaf
  • 553
  • 5
  • 13
1

If you return a pointer or non-const reference to your private data member via a public method, it is pointless to make it private. You could make the property public as well.

This basically means you allow to manipulate the member outside of the class. IMO this is considered a bad practice, or even an anti-pattern because the class cannot depend on its own state. This is very important.

E.g: imagine you have a pointer, and somebody sets it to null. Even the private methods would need to check for it, even if internally such state is impossible to reach.

Returning normal members, such as classes is considered a bad practice because it involves copying of whole objects. Usually it is better to return a reference or probably preferably const reference.

The getters in turn would allow you to put const constraints. In both cases, pointers and references.

Also please note, that in such cases usually, one provides two methods. compute and get. Currently you can access your member only by computing it!

I won't go so far to suggest you to switch to std::vector as I do not know what you need, and vectors are not good for everything. So sticking with pointers, this is the safe way to go:

class A
{
     private:
       double **CR;
     public:
       double const * const * compute2D();
       double const * const * getCR();
};

double const * const * A::compute2D(){
 return CR;
}

double const * const * A::compute2D(){
 /*Heave CPU stuff*/
 return CR;
}

int main(){
A a;
double const* const* tmp = a.compute2D();
tmp[1][2] = 0; //this will fail to compile
tmp[1] = 0; //this will fail too

double get_test = tmp[1][2]; // this passes!
}

Notice double const qualifiers. It is important to protect each level of pointer references.

luk32
  • 15,812
  • 38
  • 62
1

Returning a class member is alright. However in your case I would do it slightly different. You return pointers to a 2D vector. This way you can return the result of you calculations you don't have to make a copy of the data. And since it returns a const value you are sure the caller will not be able to change the data, but can use it to read the values.

Class B
{
     private:
       vector<vector<double> > CR; /*note space between > > is significant*/
     public:
       const vector<vector<double> >& compute2D(){ 
            /*do calculation here eg.*/
            return CR;
       }
};

And since my example show this how you can do this using std::vector you don't have to wory about deleting the dynamically reserved memory. You must make sure the instance of Class B is not destroyed eg by going out of scope if you are still using the reference variable.

hetepeperfan
  • 4,292
  • 1
  • 29
  • 47
  • AFAIK, it used to be significant due to ambiguous/buggy parsing rules. I don't remember if standard was not precise enough or just gcc buggy, as I recall visual studio compiler worked fine. However, better safe than sorry! – luk32 Jul 29 '13 at 13:19
  • @luk32 gcc still parses it as operator `>>` if the space is omitted. myversion of g++ says `error: ‘>>’ should be ‘> >’ within a nested template argument list` if I dont use it. (Although not when compiling with --std=c++0x) new for me;-) – hetepeperfan Jul 29 '13 at 13:26
  • Oh, c++0x is passe! We're in `c++11`! Btw. I guess it was the standard's "fault" if changing mode changes the semantics. – luk32 Jul 29 '13 at 13:37
  • @luk32 yep I know it is C++11, but my compiler really doesn't accept --std=c++11 maby more modern versions of gcc do. – hetepeperfan Jul 29 '13 at 13:41
1
  1. it is bad because you are returning private variable with writing access, maybe it is not so private
  2. it is bad bacause you are returning a pointer, in this case it is not clear who has the responsibility to free the memory: the class because it is its own member or the user since he call the method?
Ruggero Turra
  • 16,929
  • 16
  • 85
  • 141
0

Reterning a class method is perfectly fine if you want that class member to be readable outside the class, which is completely dependent upon what you're trying to accomplish. As for using a getter method, you already are; it is "good practice" to use such methods to create a more uniform, encapsulated setup for your class and therefore make it easier to edit the class later. Even if your function does other things (in this case, computes something in two dimensions), it's still a getter method.

In this particular case, however, you're not returning a value; you're returning a double pointer to a value. This means that the user will be able to edit the data (which I'm guessing is an array) ouside the class, which is NOT a good thing. You should perform a deep copy of the data before returning it.

As for using the garbage collector correctly: yes, you are. Although it's not called a garbage collector in C++, it's simply called freeing memory; a garbage collector is the name for a system that frees memory automatically.

IanPudney
  • 5,941
  • 1
  • 24
  • 39
  • Ahh yess I see. Thank you you for your question @quinxorin. So even with the return getter function, an outside user can modify right? And if i return a const pointer. It means that It cannot be modified – Hani Goc Jul 29 '13 at 12:59
  • 1
    I disagree. In general case it is not a good idea. Do you really think that returning something like `std::vector< std::vector > big_table` is a good idea ? It might be fine only for small basic types, such as numeric types. Maybe strings, at most. – luk32 Jul 29 '13 at 13:10
  • Return the data structure by reference (const or not) and then it is up to the caller to decide whether he wants a copy of it. – Neil Kirk Jul 29 '13 at 13:37
  • @NeilKirk That is why this answer is at least very missleading. – luk32 Jul 29 '13 at 13:41