Compilation error for string pointer in class

Question

I am new to classes and constructors. This program requires user to input name for two circles. I defined a default constructor to set parameters for radius and name and another constructor to accept them as arguments. I believe there is an issue with setName and also it tells me the constructors have already been defined. Any help is appreciated!

#include <iostream>
#include <cstring>
#include <string>
using namespace std;

class Circle
{
private:
    double pi = 3.14;
    double radius;
    string *name;

public:

    Circle();

    Circle(double, string);

    Circle::Circle()
    {
        radius = 0.0;
        *name = nullptr;

    }

    Circle::Circle(double r, string n)
    {
        radius = r;
        *name = n;
    }

    ~Circle()
    {
        delete[] name;
    }

    void setRadius(double r)
    {
        if (r >= 0)
            radius = r;
        else
        {
            cout << "Invalid radius\n";
            exit(EXIT_FAILURE);
        }
    }

    double getRadius()
    {
        return radius;
    }

    double getArea()
    {
        return pi* radius * radius;
    }

    double getCircumference()
    {
        return 2 * pi * radius;
    }

    void setName(string n)
    {

        *name = n;
    }

    string getName()
    {
        return *name;
    }

};


int main()
{
    Circle circle1;
    Circle circle2;
    double circRad1;
    double circRad2;
    string name1;
    string name2;

    cout << "Enter the name for circle 1: ";
    getline(cin, name1);

    cout << "Enter the name for circle 2: ";
    getline(cin, name2);

    cout << "Enter the radius for cirle 1: ";
    cin >> circRad1;

    cout << "Enter the radius for cirle 2: ";
    cin >> circRad2;

    circle1.setRadius(circRad1);
    circle2.setRadius(circRad2);
    circle1.setName(name1);
    circle2.setName(name2);


    cout << "Circle 1 name: " << circle1.getName() << "\n";
    cout << "Circle 1 radius: " << circle1.getRadius() << "\n";
    cout << "Circle 1 area: " << circle1.getArea() << "\n";
    cout << "Circle 1 circumfrence: " << circle1.getCircumference() << "\n";
    cout << "\n";

    cout << "Circle 2 name: " << circle2.getName() << "\n";
    cout << "Circle 2 radius: " << circle2.getRadius() << "\n";
    cout << "Circle 2 area: " << circle2.getArea() << "\n";
    cout << "Circle 2 circumfrence: " << circle2.getCircumference() << "\n";

    return 0;
}

Answer 1

Problems I see:

Constructors

You have:

Circle();

Circle(double, string);

Circle::Circle()
{
    radius = 0.0;
    *name = nullptr;    
}

Circle::Circle(double r, string n)
{
    radius = r;
    *name = n;
}

That is not correct since the first two lines declare the constructors while you declare, and define, them again with incorrect syntax.

Remove the first two lines.

Use of name

It's not clear why you are using string* for name. Make it an object, not a pointer.

string name;

Then, change the constructors to:

// Use the default constructor to initialize name
Circle() : radius(0.0) {}

Circle(double r, string n) : radius(r), name(n) {}

You may remove the destructor altogether. If you insist on having one, change it to (there is no need for delete name any more):

~Circle() {}

Change setName() to:

  void setName(string n)
  {
     name = n;
  }

Change getName() to:

  string getName() const
  {
     return name;
  }

PS Your attempted code indicates to me that you will benefit from going through the fundamentals of the language from a good book. See The Definitive C++ Book Guide and List for ideas.

Answer 2

I just want to add to the previous answer since I don't have enough points to comment.

In his constructor, he's using what's called an initializer list:

i.e.

Circle() {
    foo = 0;
    bar = 0;
}

versus (initializer list):

Circle() : foo(0), bar(0) {}

When you're just initializing variables, the preferred practice is almost always the list format. This is because it provides the arguments to construct your object before it is instantiated. This would allow you to construct objects whose "identity" isn't known until runtime (i.e. a variable-type object, or an object which could take on more than one type, although these aren't quite native to c++), or a const value.

I am a little suspicious about your instructor having you create dynamic objects in your first programming course, but since this seems to be the case,

The reason you can't simply pass in a string object and assign the pointer to it is because a pointer is strictly an address to an already existing object. This would only work if the string were passed in by reference, and then the syntax might be:

void foo(std::string& str) {

    name = &str;
}

When you pass in by value (without the ampersand) a copy of your object is being made to pass in via the parameter. This copy doesn't exactly have it's own home in memory yet, and it's definitely not the same home as what you passed in the parameter. So when you try to give it's address to the pointer, the compiler wants to complain because the address you're trying to save is going to disappear as soon as this scope is over with (the next } is hit).

You can, however, create a permanent object with a copied value. This is when you allocate dynamic memory on heap (normally it's on the stack). This would look something like:

void foo(std::string str) {

    name = new std::string(str);
}

This will allow your name pointer to point to a newly created object on the heap. This is why you need the delete[] expression in your deconstructor, because the compiler can't manage dynamic memory for you, so you have to make sure to free it before the program ends.

Note that the [] are needed because a string is actually an array of characters. When you dynamically allocate an array, the [] notation will ensure that the memory until a sentinel value is read is freed. A sentinel character almost always refers to NULL or 0 on the ASCII chart.

If it were an int being freed, the syntax would just be:

delete x;

One last note. In your private section you have a variable called pi which is default initialized to 3.14. This is presumably because this is a value which will often be referred to and is common amongst all circles. When you have such common variables which will be the same in every instance of that class, you'll want to use what are called static variables. This is a variable which is allocated once, and which everyone associated with that variable has access to. Also, because you don't want pi to change, it should be const. It might look like this:

private:
    static const double PI = 3.14;

What this will do is create one object called PI, and that exact same PI object will be used in every single circle you create. This will vastly cut down on the memory usage of that object, assuming you may create many. It is also good to note that typically const variables are capitalized, and non-const variables are not.

Answer 3

I agree with all of the points made by @RSahu, but will attempt to answer your specific issues.

Disclaimer: Using pointers as you do in this assignment is unnecessary and dangerous. It is even more unusual to require the use of them in this situation as pointers are a notoriously difficult concept for beginners to grasp.

Defining Constructors

You are defining each constructor twice.

Circle();

Circle::Circle()
{
    // ...
}

and then

Circle(double, string);

Circle::Circle(double r, string n)
{
    // ...
}

You only need to define them once. If you are declaring and defining them at the same time then the following is sufficient:

Circle()
{
    // ...
}

If you want to declare and define them separately then you can do:

class Circle
{
public:

    // Declare the constructor
    Circle();

};

// Then later in some source, define it
Circle::Circle()
{
    // ...
}

Implementing Constructors

You have crucial mistakes in both constructors (ignoring the fact that you are forced to use string*).

First,

Circle()
{
    radius = 0.0;
    *name = nullptr;
}

When you perform *name = nullptr you are dereferencing the name pointer and assigning it to nullptr.

This is bad for multiple reasons:

1. name has not been set. You are dereferencing a garbage pointer and setting it to nullptr. This is a crash.

2. Even if name had been initialized, you are setting the string object that it points to to nullptr which is another crash.

The proper way to initialize this would be as:

Circle()
    : radius{ 0.0 },
      name{ nullptr }
{

}

Let us look at the other constructor now.

Circle(double r, string n)
{
    radius = r;
    *name = n;
}

Again, radius is set correctly (mostly) but we have major issues with name.

1. name once again is uninitialized. So we are setting a non-existant string that name points to to n.

Now, here we are actually spared a bit of good luck. If you instead were performing

name = &n;

Then that would be bad as n is a temporary object. Once we leave the constructor our name would be pointing to garbage and you would crash the next time you try to access it.

But so how do we fix this constructor? I would do it like so:

Circle(double const r, string n)
    : radius{ r },
      name{ new string{n} }
{

}

In name{ new string{n} } we are setting name to a new string object that is initialized by the value in n.

Hope you are beginning to understand why in my disclaimer I do not approve of the requirement of using string* ...

Fixing setName

So, your implementation of setName is almost OK.

If we created an object of Circle using the second constructor it would be fine. Our string that name points to would simply be set the value of n.

But what if we are using a Circle created via the first constructor? Then we would be dereferencing a nullptr and attempting to set it the value of n. Crash.

I would actually fix this problem in your first constructor by changing it to:

Circle()
    : radius{ 0.0 },
      name{ new string }
{

}

So now we know name always points to a valid string object.

Finally, the Destructor

In the destructor you are using the incorrect delete[].

Use delete[] when deleting a dynamic array of objects. string is a single object, and thus should use delete.

I personally also think it is a good habit to set any deleted pointer to nullptr so that any common nullptr checks will work and not fail due to garbage.

~Circle()
{
    delete name;
    name = nullptr;
}