Using a vector data structure - design and syntax questions

Question

I have some basic C++ design/syntax questions and would appreciate your reply.

• I have N number of regions
• Each region needs to store information about an object "element"

i.e. I want to achieve something like this:

region[i].elements = list of all the elements for region i.

Question 1: Does the following syntax (see code below) / design look correct. Am I missing anything here?

EDIT

The instances of struct elem are created by some other class and its memory deallocation is handles by that class only I just want to access that object and its members using reg[i].elements list (vector) ... so, how should I add these element objects to the vector "elements" in class Region?

//Already have this stucture that I need to use

struct elemt {
  int* vertex; 
  int foo1;
  double foo2;
};


class Region{
   public:
      // I am not sure what should be the syntax here! 
      // is it correct?
      std::vector <elemt*> elements;      
}

// Following is the constructor of "class A"
A::A(){
    // --header file defines: Region *reg;
    // Let numOfRegions be a class variable. ( changes based on "Mac"'s suggestion)
    numOfRegions = 100;
    //allocate memory:
    reg = new Region[numOfRegions];
}

A::~A(){
  delete [] reg;
  reg = NULL;
}

A::doSomething(){

// here I want to append the elements to the vector
// Let i be region 10. 
// Let e1 be an element of "struct elemt" that needs to be added

  reg[i].elements.push_back(e1);

}

Question 2: Is the syntax in doSomething() correct? Later I want to run an iterator over all the elements in reg[i] and want to access, e1->foo1, e1->foo2 and like that.

Question 3: In do something method, how do I ensure that e1 is not already in the "elements"

UPDATE

Corrected some syntax errors, and hopefully fixed memory leak noticed by user 'Mac. '

Answer 1

First of all get rid of the memory leak from the code.

A::A(int numOfRegions = 100){

    m_reg = new Region[numOfRegions]; // define Region *m_reg in the class 
}

A::~A(){
  delete [] m_reg;
  m_reg = NULL;
}

You are allocating memory in the constructor and storing return address in local variable and it ll get destroyed when its scope is over .

You should store the base address so that you can delete it .

Answer 2

How do you know if two elements are same or not?

From your question about detecting if e1 is already there in elements, It seems like a map/set/hash might be a better data structure than a vector.

Also, I believe it is push_back, not pushBack.

Answer 3

You should detach the syntax from the design question. Syntax is something the compiler will check, and enforce, but then again you might get unwanted semantics...

Focusing on the design, what are your requirements? (Sorry I tend to read diagonally if there are more than a couple of lines, so I might have missed something there)

There is a class that creates/destroys elemt objects, is dynamic allocation really a requirement? or is it just because the elements are constructed with data available in the constructing class? If dynamic allocation is a requirement, then it looks like elemt objects should be held by pointer (probably smart pointers). Does the class that is meant to delete the objects keep track of them, or does it just handle the objects to the outside world expecting user code to call a deallocator function? Are the elemt objects shared among different entities? And there are other issues that you should consider...

Maybe you should try to reword your question, the code you posted basically is syntactically correct (just check with a compiler, I did not), but without knowing what you really want to achieve, you cannot really ask whether that is a good or bad design. Describe your real problem and then ask whether the code can be a solution to your requirements.

Answer 4

As you ask for syntax, I can spot a missing semi-colon right away.

Also, public data (such as elements) is not really recommended. Instead, try to use getter and setter functions.

The numOfRegions variable aught to be const int, not just int. Also, feels off to declare it in the c'tor instead of in the class declaration, as it is a tuneable setting. As soon as you want to iterate over reg, you'll need to redeclare it if you keep it as now.

As for question three, you will simply have to check to see if you can find it. You will probably need an bool operator==(const elemt &, const elemt &) to be able to do that conveniently.