overloading the extraction operator >> in C++ [duplicate]

Question

Possible Duplicate:
Operator overloading

I have to code a clock program in which I could enter the hours, minutes and seconds while overloading the extraction operator. These are my codes:

clockType.h

#include<iostream>
using namespace std;

class clockType
{
public:
   clockType();
   void getTime();
   friend istream& operator>>(istream&, const clockType);
private:
   int hr, min, sec;
}

clockType.cpp

#include<iostream>
#include'clockType.h"
using namespace std;

clockType::clockType()
{
    hr = 0;
    min = 0;
    sec = 0;
}

void clockType::getTime()
{
    while(hr>=24)
        hr = hr - 24;
    while(min>=60)
        min = min - 60;
    while(sec>=60)
        sec = sec - 60;
    cout<<setfill('0')
        <<setw(2)<<hr<<":"
        <<setw(2)<<min<<":"
        <<setw(2)<<sec<<endl;
 }

 istream& operator>>(istream& in, clockType cl)
 {
    in>>cl.hr>>cl.min>>cl.sec;
    return in;
 }

entryPoint.cpp

 #include<iostream>
 #include'clockType.h'

 using namespace std;

 int main()
 {
   clockType clock;
   cout<<"Enter hr, min, sec";
   cin>>clock;
   clock.getTime();
   return 0;
 }

There is no error. My question is, as I enter the hr, min and sec, why does it output 00:00:00? Why doesn't the >> pass its values to the object clock?

Answer 1

The signature of the operator>> needs to be

istream& operator>>(istream& in, clockType& cl) 

That is, it should accept a reference to a clockType instance.

Your current code accepts a clockType instance so when the operator is invoked a temporary copy of your clock is made and the operator works on that. The copy is then discarded, and your original clock remains unmodified.

Another issue here is that you are not checking if you successfully read anything from your input stream. Any and all of the >> operations on in could fail, which could leave cl in an unknown state. So first of all you should test for success with something like if(in >> cl.hr).

That's still not enough because the first read operation (into hr) could succeed, but the next one could fail; that will leave your cl in an unknown state. It would be good if the extraction operator had transaction semantics, i.e. either it updates all three members or otherwise it leaves the object in its previous state. One way to do that would be to read into local variables first, and only if all three reads succeed copy the values into cl.

Answer 2

Your extraction operator should probably change the object you passed to it rather than just the copy of it. That is, you want to make the second argument of your extraction operator a non-const reference, too:

std::istream& operator>> (std::istream& in, clockType& cl) { ... }

BTW, always check that the extraction worked before using the results:

if (std::cin >> clock) { ... }