Setting char* properties of a class

Question

I have 10 char* properties of my class called Car,what is the best way to write the setters of these 10 char* properties? One way is to directly set the value in it :

void Car<T>::setKey(const char* toCopyKey)
{
   delete[] key;
   int length=strlen(toCopyKey);
   key=new char[length+1];
   strcpy(key,toCopyKey);
}

and do this 10 times ,other solution I thought of , is to make a function that creates a copy of the passed char* and then assigns it in the setter :

char* Car<T>::copyString(const char* s)
{
    int length=strlen(s);
    char* property=new char[length+1];
    strcpy(property,s);
    return property;
}

and use the copyString method in every setter like this :

void Car<T>::setModel(const char* toCopyModel)
{ 
    delete[] model;
    model=copyString(toCopyModel);    
 }

But I was wondering if this second solution is correct and if there is a better way to do this copying?I cannot use std::string and vector.

The second solution seems correct, for a version of c++ with a hamstrung standard library. Beware that is `new` throws you have no exception safety and are left with a dangling `model`. — François Andrieux, May 25 '18 at 21:21
Why are you allowed to use something as advanced as templates, but not use something as basic as `std::string`? — Remy Lebeau, May 26 '18 at 00:58

score 1 · Answer 1 · edited May 26 '18 at 01:02

I guess this is an assignment of some C++ course or tutorial, because otherwise I would recommend to question the whole design.

In general, I would learn as early as possible to not do manual memory management at all and use C++ standard library smart pointers. This relieves you from the burden to write destructors, copy|move-assignment and copy|move constructors.

In your example, you could use std::unique_ptr<char[]> to hold the string data. This is also exception safe and prevents memory leaks. Creation of the unique_ptr<char[]> objects can be centralized in a helper method.

class Car {
private:
    std::unique_ptr<char[]> model;
    std::unique_ptr<char[]> key;

    static std::unique_ptr<char[]> copyString(char const* prop) {
         auto const len = std::strlen(prop);
         auto p = std::make_unique<char[]>(len+1);
         std::copy(prop, prop + len, p.get());
         p[len] = '\0';
         return p;
    }

public:
    void setModel(char const* newModel) {
         model = copyString(newModel);   
    }

    void setKey(char const* k) {
        key = copyString(k);
    }

    char const* getModel() const {
        return model.get();
    }
};

If you don't know them, I would recommend to read about the rule of zero.

Do I still have to delete the unique_ptr if I use assignment operator and also in the destructor? — Karmen, May 25 '18 at 22:38
@Karmen no, all of that is taken care of for you by the compiler. — Remy Lebeau, May 26 '18 at 01:00

score 0 · Answer 2 · edited May 25 '18 at 21:55

0

You can combine your two methods by using a reference parameter:

static void Car<T>::setStringProp(char *&prop, const char *toCopyString) {
    delete[] prop;
    prop = new char[strlen(toCopyString)+1];
    strcpy(prop, toCopyString);
}

void Car<T>::setModel(const char *toCopyModel) {
    setStringProp(model, toCopyModel);
}

And make sure that your constructor initializes all the properties to NULL before calling the setters, because delete[] prop requires that it be an initialized pointer.

Car<T>::Car<T>(const char *model, const char *key, ...): model(nullptr), key(nullptr), ... {
    setModel(model);
    setKey(key);
    ...
}

edited May 25 '18 at 21:55

Jens

9,058
2
26
43

answered May 25 '18 at 21:23

Barmar

741,623
53
500
612

Thanks Barmar,I tried to use it ,however sometimes it works,sometimes the console crashes .All I do is make an object with parameters and in the constructor is where I use the setters which call the setStringProp function. – Karmen May 25 '18 at 21:40
1

Does your constructor initialize all the properties to `nullptr`? `delete[] prop;` requires that the property be a null pointer or a previously allocated pointer. – Barmar May 25 '18 at 21:42
It doesn't ,it directly uses the setter and my properties are declared as for example char* key; – Karmen May 25 '18 at 21:43
You need to initialize the property to `nullptr` before calling the setter. – Barmar May 25 '18 at 21:44
The same is true of your original versions. – Barmar May 25 '18 at 21:45
Is it ok,to remove the delete[] from the setStringProp function and use that function directly in the constructor and where needed to write delete[] and then use setStringProp method – Karmen May 25 '18 at 21:45
You could do that, but the whole point of putting it in the function is so that you don't have to remember to do it before every call to `setXXX`. – Barmar May 25 '18 at 21:47
The only special case is when you're initializing the properties, so handle that specially by assigning `nullptr` first. – Barmar May 25 '18 at 21:48
So initializing all char* properties in the header declaration as char* key=NULL ; will do the work? – Karmen May 25 '18 at 21:49
I've added what it should look like to the answer. – Barmar May 25 '18 at 21:52
Is there a difference between model(NULL) in the constructor and char* model=NULL in the header file of the class? – Karmen May 25 '18 at 21:55
Use `nullptr` instead of `NULL` . – Jens May 25 '18 at 21:57
@Karmen See https://stackoverflow.com/questions/11594846/default-member-values-best-practice?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa – Barmar May 25 '18 at 21:58
1

Also note that this solution is not exception safe. When the allocation in `setString` throws your class will be in an inconsistent state because the member has been deleted. You could fix that with a temporary which is swapped with the member before deletion: `char* x = new char[strlen(toCopyString)+1]; strcpy(x, toCopyString); std::swap(x, prop); delete[] x;` – Jens May 25 '18 at 22:08

Setting char* properties of a class

2 Answers2