Error when creating a string in a function of class

Question

I create a class named Employee, in private, I have a Name as a string . here is my class declaring:

class Employee
{
     string Name;
 public:
     Employee();
     void SetName(string);
     void StringToEmployee(string);
     ~Employee();
 }

this is definition of StringToEmployee(string) method:

void Employee::StringToEmployee(string s)
{
     char  *first = s, *end = s+strlen(s), *last = NULL;
     last = find(first, end, ',');
     string temp(first, last- first);
     SetName(temp);
}

The error occurs when I debug to the line string temp(first, last- first), it's seem to the compiler does not allow me to construct a new string in method. cause I have also changed into string temp; then temp.assign(first, last-first). the error still remain. How could I create a new string in a method?

The [`std::strlen`](http://en.cppreference.com/w/cpp/string/byte/strlen) function is a C function. It needs a C null-terminated byte string as input. I.e. a pointer to `char`. Try using the [`std::string` functions](http://en.cppreference.com/w/cpp/string/basic_string) instead. — Some programmer dude, Nov 30 '16 at 12:59
What on earth do you have `string` defined as in the first place? Where is the rest of your [MCVE](https://stackoverflow.com/help/mcve)? And, _what_ is the error? — Useless, Nov 30 '16 at 14:38

score 1 · Answer 1 · edited May 23 '17 at 12:13

You should be using iterators and taking advantage of the features of the standard library, rather than raw pointers and C-style string functions. Not only will this give you more idiomatic and easier to understand C++ code, but it will also implicitly resolve many of your errors.

First, the implementation of StringToEmployee should be rewritten as follows:

void Employee::StringToEmployee(std::string s)
{
    const std::string temp(s.begin(),
                           std::find(s.begin(), s.end(), ',');
    SetName(temp);
}

But since you are not modifying the s parameter and do not need a copy of it, you should pass it by constant reference:

void Employee::StringToEmployee(const std::string& s)
{
    const std::string temp(s.begin(),
                           std::find(s.begin(), s.end(), ',');
    SetName(temp);
}

Also, you should consider redesigning your Employee class. Currently, you have a default constructor that creates an invalid Employee object, and then you have member functions that allow you to turn that invalid Employee object into a valid one by settings its members. Instead, you could have a constructor that did all of this initialization for you, in one step. Not only would your code be cleaner and easier to understand, but it would be more efficient, too!

Perhaps something like:

class Employee
{
     std::string Name;                          // name of this employee

 public:
     Employee(const std::string& name);         // create Employee with specified name
     void SetName(const std::string& newName);  // change this employee's name
     ~Employee();
 };



Employee::Employee(const std::string& name)
    : Name(s.begin(), std::find(s.begin(), s.end(), ','))
{ }

void Employee::SetName(const std::string& newName)
{
    Name = std::string(s.begin(), std::find(s.begin(), s.end(), ','));
}

Employee::~Employee()
{ }

A couple of quick notes:

You'll see that I always explicitly write out std:: whenever I use a class from the standard library's namespace. This is a really good habit to get into, and it's not really that hard to type an extra 5 characters. It's particularly important because using namespace std; is a really bad habit to get into.
I pass objects (like strings) that I don't need to modify or have a copy of inside of the method by constant reference. This is both easier to reason about, and also potentially more efficient (because it avoids unnecessary copies).
Inside of the constructor, I have used what may appear to be a funny-looking syntax, involving a colon and some parentheses. This is called a member initialization list, and it's something you should get used to seeing. It's the standard way for a class's constructor to initialize its member variables.

score 0 · Answer 2 · answered Nov 30 '16 at 13:02

For some reason you want to assing std::string to char*. Judging from other your code, you want to work with raw char array, so, you need to put correct pointers to first and last like this:

char *first = &s[0], *end = (&s[0]) + strlen(s.c_str()), *last = NULL;

And this part:

string temp(first, last- first);

is incorrect, because last - first is pointer, and, as I understand, you want to use std::string(const char*, size_t) constructor. But instead, you are using iterator-based constructor and system is correctly dying, because first pointer is larger, than second one.

As you see, your method is error-prone. I recommend re-do this part of code, using iterators, like this:

void Employee::StringToEmployee(string s)
{
    auto found = find(s.begin(), s.end(), ',');
    string temp(s.begin(), found);
    SetName(temp);
}

Error when creating a string in a function of class

2 Answers2