0

I want to have a class which has a member array. The size of the array should be given when I initialize the object. I just found a way with pointers to do this. I think it is working correctly, but can you maybe tell me if this is the best way to do it or if there are any things which do not work which I haven't recognized yet?

#include <iostream>
using namespace std;

class Surface {
  private:
    float dx;
    int N;
    float* mesh_points;

  public:
    Surface(float, int);
    ~Surface();
    void set_dx (float);
    float get_dx();
};

Surface::Surface(float dx,int N){
  this->dx = dx;
  this ->N = N;
  mesh_points = new float[N];
}


void Surface::set_dx (float dx) {
  this->dx = dx;
}


float Surface::get_dx (void) {
  return dx;
}

Surface::~Surface(){
  delete[] mesh_points;
}

int main () {
  Surface s(1.2,10);
  s.set_dx (3.3);
  cout << "dx: "<< s.get_dx() <<endl;

  float mesh_points[3];
  return 0;
}
Improve this question
11
  • 4
    Best way to do is start using std::vector. It give you a facility to use dynamic size array. Also it does all the optimized memory mgmt while resizing itself. Commented Sep 17, 2018 at 10:08
  • 5
    std::vector exists entirely for this purpose - a dynamically sized array that does all the memory management for you (as it's notoriously fiddly to get right). Commented Sep 17, 2018 at 10:08
  • 2
    In addition to what was stated: your class lacks copy-constructor, and copy-assignment operator. If your class instances are ever assigned to each other, or copied, you would invoke undefined behavior due to double-freeing the same memory. Commented Sep 17, 2018 at 10:12
  • 2
    Your code fails to follow the rule of three so, for instance, a simple program like int main() { Surface s(1.2,10); Surface t(s); } will fail. Like others have said the easy way to do this is to use std::vector. Commented Sep 17, 2018 at 10:12
  • 1
    Also, you may wish to reconsider your use of what are often considered bad practices: using namespace std; and endl (another for endl) (those are links to explanations). Commented Sep 17, 2018 at 10:14

2 Answers 2

Reset to default
5

can you maybe tell me if this is the best way to do it or if there are any things which do not work which I haven't recognized yet?

That'd be my take basing on existing best practices:

class Surface {
private:
    std::vector<float> mesh_points;

public:
    float dx;

    Surface(float dx, std::size_t n);
};

Surface::Surface(float dx, std::size_t n)
  : dx(dx)
  , mesh_points(n)
{
}

In short, the changes made:

Please note that the current interface doesn't allow any access to mesh_points.

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

1 Comment

For consistency , the type of the second argument of the constructor would be better specified as the same type used by std::vector for sizes. Formally, that is std::vector<int>::size_type. Practically it is often std::size_t. It is never a signed type, such as int.
0

Here's an alternative suggestion that allows you to keep your current implementation but is a lot safer.

class Surface {
  private:
    float dx;
    int N;
    float* mesh_points;

  public:
    Surface(float, int);
    ~Surface();
    void set_dx (float);
    float get_dx();
    Surface(const Surface&) = delete;            // new
    Surface& operator=(const Surface&) = delete; // new
};

By deleting the implementation of the copy constructor and copy assignment operator you prevent your Surface objects from being copied (which would likely crash your program anyway). Any attempt to copy Surface objects will now result in a compile time error.

Only a suggestion, my first choice would always be to use std::vector.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.