Code Restructure Help

This is a discussion on Code Restructure Help within the C++ Programming forums, part of the General Programming Boards category; ==> class MessageAssembler { This function has over 60 lines. You need to split it into smaller functions ==> At ...

  1. #1
    Registered User
    Join Date
    Aug 2006
    Posts
    127

    Code Restructure Help

    ==> class MessageAssembler { This function has over 60 lines. You need to split it into smaller functions
    ==> At least one function in your code must use 'call by reference'. You do not appear to have done this in your program.

    Can anyone help me with restructuring my code so that it makes my tutor happy? I'm a little wary of changing it on my own as it works perfectly now it just isn't formatted how she wants it.

    Code:
    #include<iostream>
    #include<fstream>
    #include<list>
    #include<string>
    #include<iomanip>
    #include<string.h>
    #include<cstdlib>
    
    using namespace std;
    
    class Packet {
       
       int sequenceNumber;
       string content;
       
       public:
       
       Packet(int sequenceNumber, string content)
       {   
          this->sequenceNumber = sequenceNumber;
          this->content = content;
       }
       
       int getSequenceNumber()   
       {
          return sequenceNumber;
       }
       
       string getContent()   
       {
          return content;
       }
    };
    
    class Message {
       
       list<Packet> pList;
       int id;
       
       public:
       
       Message(int id)   
       { 
          this->id = id;
       }
       
       ~Message()   
       {
          pList.clear();
       }
       
       int getID()   
       {
          return id;
       }
       
       void print()   
       {
          
          for(list<Packet>::iterator it = pList.begin(); it!=pList.end(); it++)
             cout<<it->getContent()<<endl;
       }
       
       void addPacket(Packet pac)   
       {
          
          for(list<Packet>::iterator it = pList.begin(); it!=pList.end(); it++)   
          {
             
             if(pac.getSequenceNumber() < it->getSequenceNumber())   
             {
                
                pList.insert(it, pac);
                return ;
             }
          }
          
          pList.push_back(pac);
       }
    };
    
    class MessageAssembler {
       
       static const int BUFFER_SIZE = 100;
       
       public:
       
       static list<Message>* assembleFromFile(char *fileName)   
       {
          
          FILE *file;
    
          file = fopen(fileName, "r");
          
          if(file==NULL || ferror(file)) 
          {
             cout<<"ERROR - Unable to open message file to read"<<endl<<endl;
             exit(0);
          }
          
          char *buffer = new char[BUFFER_SIZE]; 
          
          list<Message> *mList = new list<Message>();
          
          while(1)   
          {
             
             fgets(buffer, BUFFER_SIZE, file);
             
             string line(buffer, strlen(buffer)-1);
             
             if(line == "END") break;
    
             int messageID = atoi( line.substr(0, 3).c_str() );
             
             int sequenceNumber = atoi( line.substr(4, 3).c_str() );
             
             string content = line.substr(8);
             
             bool found = false;
             
             for(list<Message>::iterator it = mList->begin(); 
                it!=mList->end(); it++)
             {
                
                if(it->getID() == messageID) 
                {
                   found = true;
                   it->addPacket(Packet(sequenceNumber, content));
                   break;
                }
                
                if(messageID < it->getID())   
                {            
                   it = mList->insert(it, Message(messageID));
                   it->addPacket(Packet(sequenceNumber, content));
                   found = true;
                   break;   
                }
             }
             
             if(!found)   
             {
                mList->push_back(Message(messageID));
                mList->back().addPacket(Packet(sequenceNumber, content));
             }
          }
          
          fclose(file);
          delete buffer;
          
          return mList;
       }
    };
    
    int main(int argc, char** argv)   
    {
       
       char *fileName;
       
       if(argc < 2) 
       {
          cout<<"ERROR - Must supply 1 argument to packet"<<endl<<endl;
          exit(0);
       }
       
       fileName = *(argv+1);
       
       list<Message> *mList = MessageAssembler::assembleFromFile(fileName);
       
       for(list<Message>::iterator it = mList->begin(); it!=mList->end(); it++)   
       {
          cout<<"Message "<<setw(3)<<setfill('0')<<it->getID()<<endl;
          it->print();
          
          cout<<endl;
       }
       
       mList->clear();
       
       return 0;
    }

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    22,115
    Quote Originally Posted by Taka
    This function has over 60 lines. You need to split it into smaller functions
    Thumbs up for your tutor. I am still recovering from the trauma of seeing a pair of team mates write a 900+ line function together (I think it topped 1000 lines before I persuaded another group member to help them break it down).

    Quote Originally Posted by Taka
    At least one function in your code must use 'call by reference'. You do not appear to have done this in your program.
    I'd say that the obvious places would be everywhere that you currently pass string and Packet objects by value. They should be passed by const reference instead.

    Quote Originally Posted by Taka
    Can anyone help me with restructuring my code so that it makes my tutor happy? I'm a little wary of changing it on my own as it works perfectly now it just isn't formatted how she wants it.
    Examine your assembleFromFile() code. Don't you think that some parts of it can be commented so the reader can better understand what it does? Now, extract those parts into a well named function, thus the function name can help the reader better understand what it does (but you can still include a comment if you feel that elaboration is needed).

    By the way: you should match new[] with delete[]. In fact, it would probably be better to make buffer into a vector<char>, then pass &buffer[0] to fgets(). (But why not use the C++ standard I/O functions?)
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #3
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    You don't need this part:
    Code:
       ~Message()   
       {
          pList.clear();
       }
    std::lists always clean up after themselves when they are destroyed.

    Also, yuck!... you're dynamically allocating, returning by pointer, and leaking a std::list. Not to mention that's inside a class with one single large static method. That kinda defats the point of using classes.

    Why don't you google "call by reference" if you don't know what it means?
    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"

  4. #4
    Registered User
    Join Date
    Aug 2006
    Posts
    127
    Thanks for your help.

    Quote Originally Posted by iMalc View Post
    Also, yuck!... you're dynamically allocating, returning by pointer, and leaking a std::list. Not to mention that's inside a class with one single large static method. That kinda defats the point of using classes.
    How would you suggest I approach it then and where would I begin changing things to make it better??

    I've changed it as below but now when I run tests on it it comes up with a Segmentation Fault error. Any ideas on where I've gone wrong so far?

    Code:
    #include<iostream>
    #include<fstream>
    #include<list>
    #include<string>
    #include<iomanip>
    #include<string.h>
    #include<cstdlib>
    
    using namespace std;
    
    int checkFile(char *fileName);
    
    class Packet {
       
       int sequenceNumber;
       string content;
       
       public:
       
       Packet(int sequenceNumber, string content)
       {   
          this->sequenceNumber = sequenceNumber;
          this->content = content;
       }
       
       int getSequenceNumber()   
       {
          return sequenceNumber;
       }
       
       string getContent()   
       {
          return content;
       }
    };
    
    class Message {
       
       list<Packet> pList;
       int id;
       
       public:
       
       Message(int id)   
       { 
          this->id = id;
       }
       
       int getID()   
       {
          return id;
       }
       
       void print()   
       {
          
          for(list<Packet>::iterator it = pList.begin(); it!=pList.end(); it++)
             cout<<it->getContent()<<endl;
       }
       
       void addPacket(Packet pac)   
       {
          
          for(list<Packet>::iterator it = pList.begin(); it!=pList.end(); it++)   
          {
             
             if(pac.getSequenceNumber() < it->getSequenceNumber())   
             {
                
                pList.insert(it, pac);
                return ;
             }
          }
          
          pList.push_back(pac);
       }
    };
    
    class MessageAssembler {
       
       static const int BUFFER_SIZE = 100;
       
       public:
       
       static list<Message>* assembleFromFile(char *fileName)   
       {
          checkFile(fileName);
    
          FILE *file;
          
          char *buffer = new char[BUFFER_SIZE]; 
          
          list<Message> *mList = new list<Message>();
          
          while(1)   
          {
             
             fgets(buffer, BUFFER_SIZE, file);
             
             string line(buffer, strlen(buffer)-1);
             
             if(line == "END") break;
    
             int messageID = atoi( line.substr(0, 3).c_str() );
             
             int sequenceNumber = atoi( line.substr(4, 3).c_str() );
             
             string content = line.substr(8);
             
             bool found = false;
             
             for(list<Message>::iterator it = mList->begin(); 
                it!=mList->end(); it++)
             {
                
                if(it->getID() == messageID) 
                {
                   found = true;
                   it->addPacket(Packet(sequenceNumber, content));
                   break;
                }
                
                if(messageID < it->getID())   
                {            
                   it = mList->insert(it, Message(messageID));
                   it->addPacket(Packet(sequenceNumber, content));
                   found = true;
                   break;   
                }
             }
             
             if(!found)   
             {
                mList->push_back(Message(messageID));
                mList->back().addPacket(Packet(sequenceNumber, content));
             }
          }
          
          fclose(file);
          delete buffer;
          
          return mList;
       }
    };
    
    int main(int argc, char** argv)   
    {
       
       char *fileName;
       
       if(argc < 2) 
       {
          cout<<"ERROR - Must supply 1 argument to packet"<<endl<<endl;
          exit(0);
       }
       
       fileName = *(argv+1);
       
       list<Message> *mList = MessageAssembler::assembleFromFile(fileName);
       
       for(list<Message>::iterator it = mList->begin(); it!=mList->end(); it++)   
       {
          cout<<"Message "<<setw(3)<<setfill('0')<<it->getID()<<endl;
          it->print();
          
          cout<<endl;
       }
       
       mList->clear();
       
       return 0;
    }
          
    int checkFile(char *fileName)
    {      
    
       FILE *file;
    
       file = fopen(fileName, "r");
       
       if(file==NULL || ferror(file)) 
       {
          cout<<"ERROR - Unable to open message file to read"<<endl<<endl;
          exit(0);
       }
       
       return 0;
    }
    Last edited by Taka; 04-18-2009 at 06:00 PM.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Extended ASCII Characters in an RTF Control
    By JustMax in forum C Programming
    Replies: 18
    Last Post: 04-03-2009, 09:20 PM
  2. Enforcing Machine Code Restrictions?
    By SMurf in forum Tech Board
    Replies: 21
    Last Post: 03-30-2009, 08:34 AM
  3. Obfuscated Code Contest
    By Stack Overflow in forum Contests Board
    Replies: 51
    Last Post: 01-21-2005, 04:17 PM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 06:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 06:05 PM

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