class member functions gone haywire.

This is a discussion on class member functions gone haywire. within the C++ Programming forums, part of the General Programming Boards category; This one is absolutely amazing. I don't get how this bug is even possible. My program tries to read in ...

  1. #1
    A10
    A10 is offline
    Registered User
    Join Date
    Nov 2006
    Posts
    85

    Unhappy class member functions gone haywire.

    This one is absolutely amazing. I don't get how this bug is even possible. My program tries to read in class data from a formatted text file. Somehow all my classes data gets changed to the same piece of data in a file. I'll explain more at the bottom

    Agent Class: (pay attention to operator>>, Thats where my problem begins.)

    Code:
    class agent
    {
          public:
                 agent();
                 agent(int idNum, string clntNm, string clntFT, string pTC, agent *next);
                 agent(const agent&);
                 ~agent(){listLength--;}
                 int getIdNum() const {return idNumber;}
                 void setIdNum(int newId) {idNumber = newId;}
                 string getClientName() const {return clientName;}
                 void setClientName(string newClientName) {clientName = newClientName;}
                 string getFileType() const {return clientName;}
                 void setFileType(string newFileType) {clientFileType = newFileType;}
                 string getPath() const {return clientName;}
                 void setPath(string newFilePath) {pathToClient = newFilePath;}             
                 agent *getNextAgent() const {return nextAgent;}
                 void setNextAgent(agent *newNextAgent) {nextAgent = newNextAgent;}
                 void setNextAgent(agent &newNextAgent) {nextAgent = &newNextAgent;}
                 void display();
                 agent operator=(agent&);
                 friend ifstream& operator>>(ifstream&,agent&);
                 friend ofstream& operator<<(ofstream&,agent*);
                 static int listLength; 
          private:
                  int idNumber;
                  agent *nextAgent; //a pointer to the next agent
                  string clientName; //its clients name
                  string clientFileType; //and its clients file type
                  string pathToClient; //where client is located on Hard Disk
    
    };
    int agent::listLength = 0;
    
    agent::agent():
                   idNumber(0),
                   nextAgent(0)
    {
                   string str = " ";
                   clientName = str;
                   clientFileType = str;
                   pathToClient = str;
    }
    //copy constructor
    agent::agent(const agent& rhs):
                       idNumber(rhs.getIdNum()),
                       nextAgent(rhs.getNextAgent()),
                       clientName(rhs.getClientName()),
                       clientFileType(rhs.getFileType()),
                       pathToClient(rhs.getPath())
                       {}
    
    agent::agent(int idNum, string clntNm, string clntFT, string pTC, agent *next): idNumber(idNum), 
    clientName(clntNm), clientFileType(clntFT), pathToClient(pTC)
    {
                        listLength++;
                        nextAgent = next;
    }//agent::agent(int,string,string,string,agent*)
    void agent::display()
    {     
         cout<<idNumber << "\t" <<clientName<< "|\t" << clientFileType << "|\t"<< pathToClient;
         cout<< "..." << endl;
    }
    agent agent::operator=(agent &agentFull) //..::!untested!::..
    {
         clientName = agentFull.getClientName() ;
         idNumber = agentFull.getIdNum();
         clientFileType = agentFull.getFileType();
         pathToClient = agentFull.getPath();
         nextAgent = 0;
         return *this;
    }
    ifstream& operator>>(ifstream& rfin,agent &rnext)
    {
          char ch;
          string s1,s2,s3,sid;
          while (rfin.get(ch)) //read a character
          {
          // cout<<"\'"<<ch<<"\'" <<endl;
           if (ch == 'a') //is it the begining of an agent
           {
            rfin.get(ch); //eat up newline
            getline(rfin,s1, '\n'); 
            getline(rfin,s2, '\n');
            getline(rfin,s3, '\n');
            cerr<<"*!*"<<s1<<"!"<<s2<<"!"<<s3<<"*!*"; //data is fine
            rnext.setIdNum(GLOBALid + 1);    //define agent
            GLOBALid++; //don't want to assign all agents with an id of 1
            rnext.setClientName(s1);  //Problem lies in these
            rnext.setFileType(s2);
            rnext.setPath(s3);  
            rnext.setNextAgent(0);
            cerr<<"!"<<rnext.getPath(); //agent is bad here
            return rfin;                                   
           }//if
          }//while
          return rfin;
    }
    ofstream& operator<<(ofstream& lhs,agent* rhs)
    {
              lhs <<'a' << '\n';   //formatting
              lhs << rhs->getClientName() << '\n';
              lhs << rhs->getFileType() << '\n';  
              lhs << rhs->getPath() << '\n';
              lhs << '\n'; //formatting
              return lhs;
    }
    Here's where I call operator>>:
    Code:
    fileManager::fileManager()
    {}
    //..::FUNCTION file2Agent()::..
    //reads a file and creates a vector of agents (not pointers to)
    vector <agent>& fileManager::file2Agent(vector<agent> &parray)
    {
          agent empty;
          empty.setIdNum(0);
          
          ifstream fin("agents.DATA");
          if(!fin) //did we get the file open?
          {
           cerr<<endl<<endl<<"\aCouldn't open file in vector <agent>& fileManager::file2Agent()"; //nope
           return parray;
          }//if
          
          while (fin >> empty) //get an agent
           {
            cerr<<empty.getPath();
            parray.push_back(empty); //put it into a vector
           }//if
           
          fin.close();
          return parray;      
    }//file2Agent
    here's the text file it reads from:
    Code:
    a
    this
    is
    testing1
    
    a
    this
    is
    testing2
    
    a
    this
    is
    testing3
    
    a
    this
    is
    testing4
    
    a
    this
    is
    testing5
    
    a
    this
    is
    testing6

    in operator>>:The program reads in the data from the file correctly but after it sets the agents values with the data the agents all get set with the first piece of data read in for that agent.

    if I display the list of agents here's what happens:
    Code:
    1 this| this|this
    2 this| this|this
    3 this| this|this
    or if the file looks like this:
    Code:
    a
    asdf
    gfs
    gfhtr
    
    a
    tye
    nf
    dfg
    
    a
    er
    vfd
    ew

    the output of the list looks like this:
    Code:
    1 asdf|asdf|asdf
    2 tye|tye|tye|
    3 er| er| er
    some how either the set functions in the agent class have gone crazy or (more likely) the get functions are all returning the same variable. I don't get how this would happen and no matter how many times I go through my code I don't get it. I've spent 3 days on this bug now and I've run out of Ideas.

  2. #2
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    There are a lot of oddities with this class.
    Whilst it is very difficult to pinpoint why what happens is happening, I can virtually guarantee that if you stop doing some of the bazaar things that your doing at the moment, and fix the serious bugs, then it will behave.

    1. There is no need to define a copy-constructor when all it does is invoke the copy-constructor on all its members. A default generated copy-constructor deos exactly the same. You shouldn't be using 'get' functions in there either, as a class knows about its own variables anyway.
    2. You have two versions of setNextAgent. There is no need for that. Remove the one that takes a reference.
    3. Your destructor decrements listLength, but your copy-constructor and default constructor do not increment it. That will cause havoc in a container. Of course if you really do need this count, then you will need to supply your own copy-constructor definition to keep accurate. However it is a very dubious thing for the class itself to know about how many instances of it there are. The conatiner it's in already tracks this.
    4. Operator = should return a reference. Again, don't use 'get' function here. I makes no sense to protect a class from itself.
    5. Your operator = does not 'copy' the nextAgent parameter, it instead sets the copy to zero. This will not play well with the container. A class such as this probably has no business keeping track of other instances in its own list inside itself either. Use a seperate list outside of the class.

    Actually come to think of it, the problem is almost certainly caused by the nextAgent pointer in this class. You cannot keep pointers to items in a vector because its resizing moves those objects.
    Remove nextAgent and listLength altogether! (and the get/set functions for it) This class should not be trying to do the job of the container.
    Last edited by iMalc; 11-06-2007 at 12:47 AM.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  3. #3
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,848
    Some things to try.
    1. Delete the "debug" and "release" directories (unless you also put some of your own data files there for convenience), then do a "rebuild all". Then retest.

    2. Create a copy of your project and start removing all the code which is not essential to showing the problem. That is the bare minimum to read the file and print the result. Keep testing as you do this.
    One of two things will happen:
    - you'll remove some apparently unrelated code and it will start working as expected. Study the part most recently removed, because it's likely that has something to do with the problem.
    - you'll reach a minimum which you can post here in it's entirety which we can compile and try for ourselves.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  4. #4
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Code:
                 void setNextAgent(agent &newNextAgent) {nextAgent = &newNextAgent;}
    This looks like begging for code like this:
    Code:
    void somefunc() 
    {
      agent a;
      ... // Code that builds agent a .. 
      setNextAgent(a);
    }
    This will link in an object that lives on the stack in your function, and will not exist any longer when you leave the function.

    Edit:
    Code:
    1 asdf|asdf|asdf
    2 tye|tye|tye|
    3 er| er| er
    This looks like a typical case of storing the address of a string rather than copying the string somewhere in your code.

    --
    Mats
    Last edited by matsp; 11-06-2007 at 03:48 AM.
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  5. #5
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,806
    Code:
    string getClientName() const {return clientName;}
    string getFileType() const {return clientName;}
    string getPath() const {return clientName;}
    Is this intended? Your use of these functions in the operator<< code would cause the same string to be output.

    Code:
    ofstream& operator<<(ofstream& lhs,agent* rhs)
    {
              lhs <<'a' << '\n';   //formatting
              lhs << rhs->getClientName() << '\n';
              lhs << rhs->getFileType() << '\n';  
              lhs << rhs->getPath() << '\n';
              lhs << '\n'; //formatting
              return lhs;
    }
    This is typically implemented (again) using references, since the object being output is not (typically) modified by the output call, it is passed as a const reference. Further, since the function is a friend of the agent class, it has access to agent's private data members and does not need the use of the agent classes get???? member functions:
    Code:
    ofstream& operator<<(ofstream& lhs,const agent& rhs)
    {
              lhs <<'a' << '\n';   //formatting
              lhs << rhs.clientName << '\n';
              lhs << rhs.clientFileType << '\n';  
              lhs << rhs.pathToClient << '\n';
              lhs << '\n'; //formatting
              return lhs;
    }
    Last edited by hk_mp5kpdw; 11-06-2007 at 07:14 AM.
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

  6. #6
    A10
    A10 is offline
    Registered User
    Join Date
    Nov 2006
    Posts
    85

    ...Wow

    1. There is no need to define a copy-constructor when all it does is invoke the copy-constructor on all its members. A default generated copy-constructor deos exactly the same. You shouldn't be using 'get' functions in there either, as a class knows about its own variables anyway.
    That does seem strange. Removed. Now display outputs the correct values (it access data directly, this means removing the get functions from this program should fix this but lets go down the list)

    2. You have two versions of setNextAgent. There is no need for that. Remove the one that takes a reference.
    these two are related
    Code:
                 void setNextAgent(agent &newNextAgent) {nextAgent = &newNextAgent;}
    This looks like begging for code like this:
    Code:
    void somefunc() 
    {
      agent a;
      ... // Code that builds agent a .. 
      setNextAgent(a);
    }
    This will link in an object that lives on the stack in your function, and will not exist any longer when you leave the function.
    Actually I realized the potential problems that this could cause when I wrote that. I wrote the second setNextAgent to work around some other code that looks like this:

    Code:
    void someFunct()
    {
     agent a;
     someOtherFunct(a,agent*);
    }
    void someOtherFunct(agent& a, agent*pa)
    {
     //build agent code
    }
    3. Your destructor decrements listLength, but your copy-constructor and default constructor do not increment it. That will cause havoc in a container. Of course if you really do need this count, then you will need to supply your own copy-constructor definition to keep accurate. However it is a very dubious thing for the class itself to know about how many instances of it there are. The conatiner it's in already tracks this.
    That was from an earlier design of this class. I forgot to remove it. I never use it in any code. Removed.

    4. Operator = should return a reference. Again, don't use 'get' function here. I makes no sense to protect a class from itself.
    I don't even need that function. I deleted it.

    5. Your operator = does not 'copy' the nextAgent parameter, it instead sets the copy to zero. This will not play well with the container. A class such as this probably has no business keeping track of other instances in its own list inside itself either. Use a seperate list outside of the class.
    The not copying the next agent parameter was intentional. operator= is gone now though.
    I've been planning to remove the agent class from the process of keeping track of itself. I think I'll do that on Friday.

    Actually come to think of it, the problem is almost certainly caused by the nextAgent pointer in this class. You cannot keep pointers to items in a vector because its resizing moves those objects.
    The vector doesn't always keep track of the agents. The nextAgent pointers are all set to 0 before entering the vector and are refilled once they leave the control of the vector. I don't use the vectors to store them all the time because I like the flexibility of switching to other containers periodically that can accomplish certain tasks better. The agents keep track of themselves like an intermediate container before switching. This seems strange to me reading over my last two sentences. Maybe I should rework this.

    1. Delete the "debug" and "release" directories (unless you also put some of your own data files there for convenience), then do a "rebuild all". Then retest.
    Will try in a moment.

    Create a copy of your project and start removing all the code which is not essential to showing the problem. That is the bare minimum to read the file and print the result. Keep testing as you do this.
    One of two things will happen:
    - you'll remove some apparently unrelated code and it will start working as expected. Study the part most recently removed, because it's likely that has something to do with the problem.
    - you'll reach a minimum which you can post here in it's entirety which we can compile and try for ourselves.
    That is a very good debugging strategy. My next post in this topic(and posts from now on) will be the results of that. :-)

    Code:
    string getClientName() const {return clientName;}
    string getFileType() const {return clientName;}
    string getPath() const {return clientName;}
    Is this intended? Your use of these functions in the operator<< code would cause the same string to be output.
    I hate myself... That was the problem. Thank you. I've checked the get functions millions of times and never saw that. Maybe the fireplace is leaking carbon monoxide. That'd give me an excuse for my pure lack of attention. Such a stupid mistake too. I don't know how they stayed like that for so long. I must not be using the get functions.

    Code:
    ofstream& operator<<(ofstream& lhs,agent* rhs)
    {
              lhs <<'a' << '\n';   //formatting
              lhs << rhs->getClientName() << '\n';
              lhs << rhs->getFileType() << '\n';  
              lhs << rhs->getPath() << '\n';
              lhs << '\n'; //formatting
              return lhs;
    }
    This is typically implemented (again) using references, since the object being output is not (typically) modified by the output call, it is passed as a const reference. Further, since the function is a friend of the agent class, it has access to agent's private data members and does not need the use of the agent classes get???? member functions:
    That originally used a reference. I think I changed it to a pointer in desperation. I forgot about how friend functions worked. Maybe I should get more acquainted with them. I'll go read a tutorial on them to refresh my memory.

    Thanks for all of your(plural your?) help.

    The post icons need an animated 2 frame smiley hitting a board on his head...
    Last edited by CornedBee; 11-08-2007 at 03:27 AM. Reason: Fixed quotes.

  7. #7
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,893
    Please fix your quotes. You use code tags instead of quote tags, thus ruining the layout.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  8. #8
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,806
    Quote Originally Posted by A10
    The post icons need an animated 2 frame smiley hitting a board on his head...
    Here you go, I used these in another recent post, they may be close to what you wanted:
    Attached Images Attached Images    
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

  9. #9
    A10
    A10 is offline
    Registered User
    Join Date
    Nov 2006
    Posts
    85

    Exclamation

    Quote Originally Posted by CornedBee View Post
    Please fix your quotes. You use code tags instead of quote tags, thus ruining the layout.
    My posts in this thread don't have edit buttons

    Here you go, I used these in another recent post, they may be close to what you wanted:
    that left one is perfect

    EDIT: Only this one does. Do the edit buttons have a time limit?
    Last edited by A10; 11-07-2007 at 07:33 PM.

  10. #10
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,893
    Possible, though I've seen people edit the code in their original post when the threads were pretty old.

    Anyway, I fixed it for you.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Help with FIFO QUEUE
    By jackfraust in forum C++ Programming
    Replies: 23
    Last Post: 04-03-2009, 09:17 AM
  2. Passing class member function to pthread_create
    By lehe in forum C++ Programming
    Replies: 6
    Last Post: 03-27-2009, 08:47 PM
  3. Using private class members in static functions
    By sethjackson in forum C++ Programming
    Replies: 2
    Last Post: 09-23-2005, 10:54 AM
  4. Problem with Visual C++ Object-Oriented Programming Book.
    By GameGenie in forum C++ Programming
    Replies: 9
    Last Post: 08-30-2005, 12:21 AM
  5. trouble defining member functions
    By dP munky in forum C++ Programming
    Replies: 7
    Last Post: 04-13-2003, 09:52 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21