Getline reading in a csv very oddly

Question

I am trying to read a csv using get line to extract three variables separated by commas. Name, Course, and Grade.

I am reading in the first line fine but it puts in weird new line breaks and sends the format into a cluster.

Here is my code :

#include "header.h"

string student::GetCourse() {
    return course;
}

string student::GetName() {
    return name;
}

string student::GetGrade() {
    return grade;
}

void student::setname(string n) {
    name = n;
}

void student::setCourse(string c) {
    course = c;
}

void student::setGrade(string g) {
    grade = g;
}
void sort (vector <student> &List) {

    student temp;
    int first = 1;
    int vectorLength = List.size() - 1;

    for (int i = vectorLength; i > 0; i--) {
        first = i;
        for (int j = 0; j < i; j++) {
            if (List[j].GetName() > List[first].GetName())
            first = j;
        }
        temp = List[first];
        List[first] = List[i];
        List[i] = temp;
    }

}

void main () {
    ifstream file;
    vector <student> StudentList;

    file.open("short.txt");

    while (!file.eof()) {

        file.ignore(8196,'\n');

        string tempname, tempgrade, tempcourse = "";

        if (file != "\n") {
            getline(file, tempname, ',');
            getline(file, tempcourse, ',');
            getline(file, tempgrade, ',');
        }

        student s;
        s.setCourse(tempcourse);
        s.setname (tempname);
        s.setGrade (tempgrade);

            StudentList.push_back(s);

    }
    //sort (StudentList);

    for (int i = 0; i < StudentList.size(); i++) {
        cout << StudentList[i].GetName() << " " << StudentList[i].GetCourse() << " " << StudentList[i].GetGrade() << endl;
    }
}

Any ideas, I am having a real hard time on reading in this file.

Answer 1

Some comments:

Don't write your own sort.

The STL has its own built in sorting algorithms.
All you do is have to specify a relationship between the objects:

bool operator<(student const& lhs,student const& rhs)
{
    return lhs.GetName() < rhs.GetName();
}
// Now a sort is:

   std::sort(list.begin(),list.end());

Don't use: while (!file.eof())

This is the standard anti-pattern for reading a file.
The problem is that it is either too early or two late for the test. If you have not read anything it is two early as nothing has happened. If you have read something then it is already too late as you have done the processing on the item you read (but failed).

The best way is to put the read into the while loop. This is because the result of the read returns a reference to the stream. This can automatically be converted into an object that can be used in a boolean context (the conversion test to see if something is wrong with the stream). So a read failure will leave the stream in a state that would cause it to be converted into the equivalent of false in a boolean context.

std::string line;
while(std::getline(file,line))
{
   // loop only entered if getline() worked.
   // Thus we know that we have a good value in line.
   // use line
}

Don't use magic numbers:

Are you really ignoring 8000 characters or just trying to drop a line?

file.ignore(8196,'\n');

You have two alternatives:

std::string ignoreLine;
std::getline(file,ignoreLine);

// Dont use a magic number but use a number that is guranteed not to fail.
file.ignore(std::numeric_limits<std::streamsize>::max(), '\n')

Don't be lazy:

The main thing about programming is writing maintainable code.
Using this kind of initialization is (relatively universally) condemned as just lazy. Put each declaration on a separate line. It makes the code easier to read.

string tempname, tempgrade, tempcourse = "";

// Like this:
std::string tempname;
std::string tempgrade;
std::string tempcourse;

Use a stringstream to break the line into parts

I am not sure what you are attempting here?

if (file != "\n")
{   getline(file, tempname, ',');
    getline(file, tempcourse, ',');
    getline(file, tempgrade, ',');
}

I think it would be easier to read if we combine it with the loop above:

std::string line;
while(std::getline(file,line))
{
    std::stringstream  linestr(line);

    if (getline(linestr, tempname, ',') &&
        getline(linestr, tempcourse, ',') &&
        getline(linestr, tempgrade, ',')
       )
    {
        // Here we have read a line.
        // And successfully retrieved three comma separated values from the line
    }
}

When opportunity arises replace a loop with a standard algorithm

This print loop can be replaced by std::copy()

for (int i = 0; i < StudentList.size(); i++)
{        cout << StudentList[i].GetName() << " " 
              << StudentList[i].GetCourse() << " " 
              << StudentList[i].GetGrade() << endl;
}

All you need to do is define an output operator for your class.

std::ostream& operator<<(std::ostream& str,student const& data)
{
    str << data.getName() << " "
        << data.getCourse() << " "
        << data.getGrade() << " "; // No newline here.
    return str;
}

Now we can copy the vector to the std::cout

std::copy(StudentList.begin(),StudentList.end(),
          std::ostream_iterator<student>(std::cout,"\n")
         );

The main bug.

The main bug I see is this line:

if (file != "\n")

Here you compare file against a 'C-string'. How the compiler manages to compile this I am not sure.
Several options spring to mind, but the fact that it is not obvious makes it a likely source of a bugs. Also note this is not how you compare two strings (unless one is a std::string).

I think the compiler will convert a file into a pointer and compare that with the "C-String" (as this is also just a pointer). You may think that is a bit strange but there is an operator that will convert a file into a void*. The pointer does not point at anything meaningful but is either NULL or not NULL and can be compared against a char* pointer thus resulting in a true (as it never equals the string "\n").

Answer 2

First: You're not checking whether input succeeds anywhere. Heck, you don't even check whether the file could be opened:

int main () {                          // it's int main()!
  ifstream file("short.txt");
  if(!file.good()) {
    std::cerr << "couldn't open \"short.txt\"\n";
    return 1;
  }

  vector <student> StudentList;
  for(;;) {
    // ...
  }
  if( !file.eof() ) {
    std::cerr << "error reading before eof!\n";
    return 2;
  }
  // ...
}

Then: Generally it's easier to first read in lines in that loop:

for(;;) {
  std::string line;
  std::getline(file, line);
  if(!file) break;
  // ...
}

and then read from those lines through a string stream. I would put the code reading in lines into its own function:

std::istream& read_line(std::istream& is, StudentList& list)
{
  std::string value1, value2, value3;
  std::getline(is, value1, ',');
  std::getline(is, value2, ',');
  std::getline(is, value3, ',');
  if(is)
    StudentList.push_back(...);
}

// ...
for(;;) {
  std::string line;
  std::getline(file, line);
  if(!file) break;

  std::istringstream iss(line);
  read_line(iss, StudentList);
  if(!iss) break;
}
// ...

HTH.

Answer 3

You've gotten a number of answers. While their suggestions are definitely improvements over what you're doing now, I'd deal with this a bit differently than they've suggested.

Right now your student class is basically doing its best to imitate being "dumb data" (i.e. just a plain struct) but with uglier syntax -- you're using a get/set pair for each member, but they're not adding anything. The student class itself is just as "dumb" as if it was just a simple struct. All the logic for the student is still outside the student class.

To make it useful, the student class should contain a fair amount of the logic involved, such as how to read a student in from a stream, or display a student on a different stream:

class student { 
    std::string name, course, grade;
public:

    bool operator<(student const &other) const {
        return name < other.name;
    }

    friend std::ostream &operator<<(std::ostream &os, student const &st) { 
        return os << st.name << " " << st.course << " " << st.grade;
    }

    friend std::istream &operator>>(std::istream &is, student &st) { 
         std::string temp;
         is >> temp;
         std::istringstream t(temp);
         std::getline(t, st.name, ',');
         std::getline(t, st.course, ',');
         std::getline(t, st.grade);
         return is;
    }
};

This makes main considerably simpler:

int main() { 
    std::ifstream in("short.txt");
    std::vector<student> students;

    std::copy(std::istream_iterator<student>(in),
              std::istream_itertor<student>(),
              std::back_inserter(students));
    std::sort(students.begin(), students.end());
    std::copy(students.begin(), students.end(), 
        std::ostream_iterator<student>(std::cout, "\n"));
    return 0;
}               

Note, in particular, that main only ever deals with a "whole" student as a logical entity -- it never once looks "inside" the student object at its component parts.

Answer 4

By setting the delimiter to be a ',' in the call

getline(file, tempname, ',');

you are not reading an entire line at a time. The '\n' is the default delimiter and by using teh default you will get the entire line not just a portion of it.

I would suggest using the default delimiter to read in a whole line and then break the line into tokens using the ',' as a delimiter and use if(!file.eof) to determine when you are finished reading teh file.

Answer 5

Well, here goes

• if (file != "\n") comparison is nonsensical. It does not do what you think it does.
• Your delimiter after the grade is not ',', it is '\n'.
• while (!file.eof()) is incorrect. That only checks the EOF after it has already happened. You should check the return value of your getline() instead

Also

• usually in C++ you do std::ifstream file("short.txt");. You don't need to call open() separately.

• You don't need to initialize std::string to "". That happens automatically. Even if you had to do that, then you should have written

  std::string a = "", b = "", c = "";.

  If you do std::string a, b, c = "something" then only c is initialized to something.