Mixing cin with >> operator for a class

Question

Hello in the following code

int main()
{
 //intialize variables

   while(true)
   {
    menu.display();
    char choice;
    cin>>choice;

      switch(choice)
      {
       case 'A':
          Car c;
          cin>>c;
          break;
      }
   }
 return 0;
}

When I input 'A' and use the overrided operator >> for Car class, menu.display() gets called 3 times but I suppose it should be called once ,where can be the problem? here is the implementation of the >> operator :

 istream& operator>>(istream &is , Car &car)
 {

    double price;
    is>>price;
    car.setPrice(price); 
    int miles;
    is>>miles;
    car.setMiles(miles);
    char brand[50];
    is.getline(brand,50);
    is.ignore();
    car.setBrand(brand);
    char model[50];
    is.getline(model,50);
    is.ignore();
    car.setModel(model);
    char category[50];
    is.getline(category,50);
    is.ignore();
    car.setCategory(category);
  return is;

}

Probably I am mixing very bad the operators but I can't understand where and how?

The biggest problem, IMO, is that you're mixing output and input in your `operator>>` function. The input operator should *only* read input, not use prompts etc. If you need prompts, create a non-operator function to read the input. — Some programmer dude, Jun 13 '18 at 22:26
Don't try to learn C++ by writing interactive programs. But if you must `is>>price;` should almost certainly be `is>>car.price;`, and similarly elsewhere. I feel a Punchtape coming on.... — , Jun 13 '18 at 22:26
Your second biggest problem, also IMO, is the use of arrays for strings instead of `std::string`. All in all it seems you could need to [get a few good books](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list/388282#388282) to read. — Some programmer dude, Jun 13 '18 at 22:27
The task requires char* ,also I have deleted the cout's from the << operator but nothing changed — Karmen, Jun 13 '18 at 22:29
Prefer to use `std::string` for text rather than character arrays. Character arrays are subject to buffer overrun errors and may contribute to memory leaks. — Thomas Matthews, Jun 13 '18 at 22:32
Prefer to use named constants for capacities. With a named constant, you'll only need to modify one location to change the value. — Thomas Matthews, Jun 13 '18 at 22:36
The `case` block is declaring a local variable. Some compilers warn/error on that if the `case` block is not surrounded in braces, eg: `case 'A': { Car c; cin>>c; break; }` — Remy Lebeau, Jun 13 '18 at 22:40
yes I will fix the style ,but I cannot figure out the multiple invocations of menu.display(); — Karmen, Jun 13 '18 at 22:42
Do you have the same problem if you reduce the code to `while(true) { menu.display(); char choice; cin>>choice; }`? — user4581301, Jun 13 '18 at 22:52
@user4581301 for this code if I input 1 char , the function gets called once ,ofcourse if I input for the char something like ABC it gets printed 3 times — Karmen, Jun 13 '18 at 22:54
@NeilButterworth Why do you say to not make interactive programs to learn C++? — David G, Jun 13 '18 at 23:03
OK. So the problem really is in `operator >>`. What is the input format? `is>>miles;` could be leaving crap like a newline in the stream to be picked off by `is.getline(brand,50);`. You know, a stroll through the code with whatever debugging software came with your development environment can probably sort this out for you fairly quickly. — user4581301, Jun 13 '18 at 23:05

Martin York · Answer 1 · 2018-06-13T23:40:17.290

Your biggest issue is that you don't validate your input.
As a result you are probably setting the input stream into an error state which results in all further read operations failing.

Your read operation should either succeed and update the car object, or it should fail and not change the state of the car (hopefully doing something so the error propagates).

First check that your user choice worked:

if (cin>>choice) {
    switch(choice)
     ...
}

Inside the read operator you need to make sure that you don't update the object until you have read the whole object from the stream.

I would do this:

 // Need a helper object to make reading lines easier.
 struct Line
 {
     char* dst;
     int   max;
     Line(char* dst, int max)
         : dst(dst)
         , max(max)
     {}
     friend std::istream& operator<<(std::istream& s, Line& line) {
         if (s.getline(line.dst, line.max) {
             s.ignore();
         }
         return s;
     }
 };

 istream& operator>>(istream &is , Car &car)
 {
    double price;
    int miles;
    char brand[50];
    char model[50];
    char category[50];

    if (is >> price >> miles >> Line(brand, 50) >> Line(model, 50) >> line(category, 50) ) {
        car.setPrice(price); 
        car.setMiles(miles);
        car.setBrand(brand);
        car.setModel(model);
        car.setCategory(category);

        // PS I hate all these setters. Leaking the implementation.
        // I would have written the above as:
        // car = Car(price, miles, brand, model, category);
    }
    // if there was a read error then `car` is unchanged
    // and the stream `is` is in an error state.
    return is;
}

Now that I have refactored your code I can bet the error is after reading the miles. I bet you have an input file that looks like this:

 price
 miles
 brand
 model
 category

Where each value is separated by a new line. The trouble is that operator>> does not read the trailing new line. So after reading miles you still have a newline on the stream. This means when you read brand it will be empty, model will be brand and category will be model. This leaves category on the stream and the next time you read a car it will fail as Category will not match a price causing the stream to go into error state.

Assuming I guessed the error correctly this can be fixed as follows:

 struct Line
 {
     char* dst;
     int   max;
     Line(char* dst, int max)
         : dst(dst)
         , max(max)
     {}
     friend std::istream& operator<<(std::istream& s, Line& line) {
         // Assume there is a proceeding newline that
         // must be removed. Do this with s.ignore()
         s.ignore() && s.getline(line.dst, line.max);
         return s;
     }
 };

 istream& operator>>(istream &is , Car &car)
 {
    double price;
    int miles;
    char brand[50];
    char model[50];
    char category[50];

    if (is >> price >> miles >> Line(brand, 50) >> Line(model, 50) >> line(category, 50) ) {
        car = Car(price, miles, brand, model, category);

        // Remove the newline after the `category`
        is.ignore();
    }
    // if there was a read error then `car` is unchanged
    // and the stream `is` is in an error state.
    return is;
}

Mixing cin with >> operator for a class

1 Answers1