1

I wanted to create a variable amount of Treasure objects when a chest object is created. This is the solution I came up with. To create an array of n Treasures, later to be parsed as needed. Room for improvement?

Chest::Chest (int n) {

Treasure * tArr = new Treasure[n];

}

Each treasure has a unique ID, and is then parsed into a map, mapping int ID to Treasure.

2
  • "Room for improvement?" Use std::vector? Commented Mar 16, 2019 at 8:21
  • 1
    If you want to later parse it, you need to store it's size somehow. You also have to implement the assignment operators, the copy and move constructor and the destructor correctly. It's probably easier to just use std::vector. Commented Mar 16, 2019 at 8:25

1 Answer 1

2

Level 0 improvement: avoid loosing the objects you've created

Your variable tArr is a local object of the constructor and will be lost when the construction is over, leading to memory leaks.

So if you want this code to be useful, you'll need to make tArr a member variable and avoid redefining it in the constructor.

Chest::Chest (int n) {
    tArr = new Treasure[n];    // assuming Treasure *tArr is a class variable
}

Level 1 improvement: the rule of 3

The first improvement would be implement the rule of 3. Otherwise, you'll quickly run into nasty troubles, if for example by accident you'd copy a Chest.

Level 2 improvement: forget about arrays and use vectors

Arrays are medieval ages to C++. Use vector instead:

class Chest {
    std::vector<Treasure> tArr;     // naming could be discussed...
    ...
}; 

Chest::Chest (int n) : tArr(n) {    // construct the vector with n elements
               // now do whatever you want with these elements
}

Now the cool thing with vector is that they can grow dynamically. So you could start with an empty vector and add new treasures using push_back():

Treasure x(...); // create a cool treasure not just default initialized 
tArr.push_back(x);  // add it at the end of the vector

This could allow you for example in your constructor to add random treasures, if uniform treasures are too boring.

But you could as well resize() it on the flow to an arbitrary value:

tArr.resize(tArr.size()*2);   // two times more treasures !!!  

Level 3 improvement: use a map

The following intent is not totally clear:

Each treasure has a unique ID, and is then parsed into a map, mapping int ID to Treasure.

Assuming ID is sequential and local to the Chest, the vector is all you need: the index will be the ID.

If however the ID is not sequential, or not local to the Chest, you may be interested in a map, which is a kind of associative array:

std::map<int, Treasure> myMap;  // maps an int ID to a Treasure. 

The problem is that you cannot just create an map with n elements in it: you need to add the elements one by one:

myMap[id] = Treasure(...);      // changes the element with id, or creates it

Cector is much simpler. So go for a map only if it's really justified.

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

Comments

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.