Cannot figure out how to get g++ to accept my code

Question

I've been debugging this code for hours trying to get the output correct and g++ error free. It was working earlier but there were logic errors in the output so then I went in and added the loop and an extra parameter in the output function.

Now g++ gives me the following error:

Student.cpp: In member function ‘void Student::InputData(std::string, int, std::string&)’: Student.cpp:81:21: error: invalid conversion from ‘std::string* {aka std::basic_string*}’ to ‘char’ [-fpermissive] /usr/include/c++/4.6/bits/basic_string.h:560:7: error: initializing argument 1 of ‘std::basic_string<_CharT, _Traits, _Alloc>& std::basic_string<_CharT, _Traits, _Alloc>::operator=(_CharT) [with _CharT = char, _Traits = std::char_traits, _Alloc = std::allocator, std::basic_string<_CharT, _Traits, _Alloc> = std::basic_string]’ [-fpermissive]

How can this code be fixed?:

//This program defines a class for storing the names of classes that a student has enrolled in.

#include <iostream>
#include <cstdlib>
#include <string>

using namespace std;

class Student
{
public:
    Student();
    Student(Student& obj); // copy constructor
    ~Student();
    void InputData(string,int,string&);     // Input all data from user
    void OutputData();      // Output class list to console
    void ResetClasses();        // Reset class list
    Student& operator =(const Student& rightSide){
    this->name=rightSide.name;
    this->numClasses=rightSide.numClasses;
    this->classList=rightSide.classList;
}
    // Assignment operator

private:
    string name;
    int numClasses;
    string *classList;
};

// --------------------------------
// ----- ENTER YOUR CODE HERE -----
// --------------------------------

// ======================
// Student::Student
// The default constructor initialized variables to empty.
// The dynamic array is intialized to NULL.
// ======================
Student::Student () {
        name="";
    numClasses=0;
    classList=NULL;


}
// ======================
// Student::Student
// The copy constructor creates a deep copy of the parameter object
// ======================
Student::Student (Student& obj) {
    obj.name=name;
    obj.numClasses=numClasses;
    obj.classList=classList;
}   
// ======================
// Student::~Student
// The destructor frees up any memory allocated to
// the dynamic array.
// ======================
Student::~Student () {
    delete classList;
}
// ======================
// Student::ResetClasses
// This method deletes the class list
// ======================
void Student::ResetClasses () {
    if(classList) { delete [] classList;}
}
// ======================
// Student::InputData
// This method inputs all data from the user. 
// It allows the user to enter an arbitrary number of classes
// using a dynamic array to store the strings.
void Student::InputData(string nm, int nClasses, string& names) {
    name=nm;
    numClasses=nClasses;
    delete classList;
    for (int i=0; i<nClasses; i++) {
        names=new string[i];
    }
}   

// Reset the class list before entering data just in case this method
// was called repeatedly and the dynamic array wasn't cleared
// ======================


// ======================
// Student::OutputData
// This method outputs the data entered by the user.
// ======================
void Student::OutputData() {
    cout << "Student name : " << name <<endl;
    cout << "Student number of classes : " << numClasses <<endl;
    cout << "Student class list : " <<classList<<endl;
}

// ======================
// Student::=

// operator, we would end up with two references to the same
// class list when the assignment operator is used.
// ======================
//
// --------------------------------
// --------- END USER CODE --------
// --------------------------------



// ======================
//     main function
// ======================
int main()
{
  // Test our code with two student classes
  Student s1, s2;

  string sname;
  int snumClasses;
  string snames[]="";

  cout << "Enter student name, number of classes, and names of classes for first student" << endl;
  cin >> sname; cin >> snumClasses; 

  int i; 
  for (i=0; i < snumClasses; i++) {
    cin >> snames[i];
  }

  s1.InputData(sname, snumClasses, snames[i]);      // Input data for student 1
  cout << "Student 1's data:" << endl;
  s1.OutputData();      // Output data for student 1

  cout << endl;

  s2 = s1;  
  cout << "Student 2's data after assignment from student 1:" << endl;
  s2.OutputData();      // Should output same data as for student 1

  s1.ResetClasses();
  cout << "Student 1's data after reset:" << endl;
  s1.OutputData();      // Should have no classes

  cout << "Student 2's data, should still have original classes:" << endl;
  s2.OutputData();      // Should still have original classes

  Student s3(s2);  // explicit copy constructor call
  cout << "Student 3's data after assignment from student 2:" << endl;
  s2.OutputData(); // should have the same classes as student 2

  cout << endl;
  return 0;
}

Answer 1

TL;DR -- http://ideone.com/rTVQUo

---

The first and foremost issue is this:

string snames[] = "";

This is declaring a array of strings that hold only one element. That will make the following code undefined behavior if snumClasses is greater than 1:

for (i = 0; i < snumClasses; i++) {
    cin >> snames[i];
}

The above code would invoke undefined behavior because you would be accessing out-of-bound addresses if i grows greater than one. Once Undefined Behavior hits your program, you can't make sense of any behavior that ensues because of this. Your program could break, your computer could shut down or worse - nasal demons.

To fix this, the size of snames should be the size that the user specifies. In particular, snames should be pointer to a dynamically-allocated array whose size is snumClasses:

std::string* snames;
std::cin >> snumClasses;

snames = new std::string[snumClasses];

^{Note: I'll explain later on why this should not be done.}

And when the data is finished being used, make it a point to delete[] it:

delete[] snames;

---

The next problem is coming from this line:

s1.InputData(sname, snumClasses, snames[i]);
//                               ^^^^^^^^^

A Student object has three data members, the name of the class, the number of classes, and the array of classes. The InputData member function is used for assigning those data members to the arguments given. You've given the correct arguments for the first two parameters (because an string can be assigned to an string and an int can be assigned to an int) but your third argument does not match the data member's type. It would make sense for you to be sending the array snames instead of the element within it which you have done by doing snames[i].

s1.InputData(sname, snumClasses, snames);
//                               ^^^^^^

This also necessitates that InputData take a string* rather than a string& and that the classList pointer simply be assigned to the new snames array:

void Student::InputData(string nm, int nClasses, string* names)
{
    name = nm;
    numClasses = nClasses;

    delete[] classList;
    classList = names;                                                         /*
    ^^^^^^^^^^^^^^^^^^                                                         */
}

---

Now that I've reduced the bulk of your errors, let's move on to improving your program.

I'll start from major apparent issues and deviate towards more minor (but important) design issues.

The copy-constructor:

Student::Student(Student& obj)
{
    obj.name       = name;
    obj.numClasses = numClasses;
    obj.classList  = classList;
}

Not only does this constructor work more like an assignment-operator, but you've switch up the assignment of the members. You're suppose to assign the data members of *this to the object being copied, not the other way around. Also, your copy-constructor has to take a reference to const:

Student::Student(const Student& obj)
{
    name       = obj.name;
    numClasses = obj.numClasses;
    classList  = obj.classList;
}

Note that a deep copy needs to be performed considering lifetime dependencies between *this and the object that's being copied.

Student::Student(const Student& obj)
{
    name       = obj.name;
    numClasses = obj.numClasses;
    std::copy(obj.classList, obj.classList + numClasses, classList);
}

One more thing wrong with this constructor is that it uses assignment instead of initialization. The data members of your class should be initialized using the member-initializer list:

Student::Student(const Student& obj)
    : name(obj.name),
      numClasses(obj.numClasses),
      classList(numClasses ? new std::string[numClasses]{} : nullptr)
{
    std::copy(obj.classList, obj.classList + numClasses, classList);
}

The assignment operator:

Student& operator=(const Student& rightSide)
{
    this->name       = rightSide.name;
    this->numClasses = rightSide.numClasses;
    this->classList  = rightSide.classList;
}

This operator has the same problem as the copy-constructor as it does consider memory management. If we are assigning from *this to rightSide, we have to delete[] the current classList object and copy the elements from the object being copied thereto. If we don't do this, we risk leaking memory:

Student& operator=(const Student& rhs)
{
    name       = rhs.name;
    numClasses = rhs.numClasses;

    delete[] classList;  // delete classList
    classList = nullptr; // set to nullptr because if 'new' throws, we can still
                         // delete without causing undefined behavior

    classList = new std::string[numClasses]{};
    std::copy(rhs.classList, rhs.classList + numClasses, classList);

    return *this; // you forgot to return the object!
}

This looks just about right, but it can also be improved. As it stands this code does not provide a strong-exception guarantee. If new throws, the state of *this will be different from when it entered into the assignment operator. To circumvent this we can apply the copy-and-swap idiom in order to provided a strong-exception guarantee:

Student& operator=(Student rhs)
{
    swap(*this, rhs);
    return *this;
}

The parameter has been changed to take by value so that when an lvalue is passed in we can copy it and swap its data members. The function swap encapsulates the swapping semantics. It is defined inside Student as follows:

friend void swap(Student& first, Student& second)
{
    using std::swap;
    swap(first.name, second.name);
    swap(first.numClasses, second.numClasses);
    swap(first.classList, second.classList);
}

For more information, see What is the copy-and-swap idiom?

---

Don't use new:

Whenever you feel like using new, consider alternatives given in the Standard Library. They implement RAII and as such don't require the user to deallocate the memory themselves. This is very advantageous as it reduces potential memory leaks and keeps your code clean and efficient. We can utilize the standard library container std::vector<T> for the list of classes and substitute it for the string* classList data member:

private:
    std::string name;
    std::vector<std::string> classList;

Notice how I've also removed the numClasses data member. We don't need it as the vector has a size() member function. Moreover, since there is no need for memory management, we can implement the Rule of Zero and not implement the default-ctor, copy-ctor and destructor! Why? Because the compiler will generate those member functions by default.

---

Use user-defined inserters/extractors

You can cut down on a lot of code by defining your own inserters/extractors. Here would be the implementation of these two:

std::ostream& operator<<(std::ostream& os, const Student& s)
{
    s.OutputData(os);
    return os;
}

std::istream& operator>>(std::istream& is, Student& s)
{
    s.classList.assign(std::istream_iterator<std::string>{is >> s.name},
                       std::istream_iterator<std::string>{});

    return is;
}

And this is how you can use it in main():

int main()
{
    Student s1, s2;

    if (std::cin >> s1 >> s2)
         std::cout << s1 << s2;
}

---

This should be the structure of your class:

class Student
{
public:
    void InputData(std::string, const std::vector<std::string>&);
    void OutputData(std::ostream&) const;
    void ResetClasses();

    friend std::ostream& operator<<(std::ostream&, const Student&);
    friend std::istream& operator>>(std::istream&,       Student&);
private:
    std::string name;
    std::vector<std::string> numClasses;
};

And this is the implementation of those member functions:

void Student::InputData(std::string nm, const std::vector<std::string>& names)
{
    name = nm;
    numClasses = names;
}

void Student::OutputData(std::ostream& os) const
{
    os << "Students name: " << name << std::endl;
    os << "Number of classes: " << classList.size() << std::endl;
    os << "Classes: ";

    std::copy(classList.begin(), classList.end(),
        std::ostream_iterator<std::string>(os, "\n"));
}

void Student::ResetClasses()
{
    classList.clear();
}