Thread: Code Restructure Help

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).

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.

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?)

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?

Thanks for your help.

Originally Posted by iMalc

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;
}