Linked File Array[C++]

Started by
9 comments, last by ShadowPhoenix 16 years, 8 months ago
I decide to embark finally on a large project. So I made a group on sourceforge, got CVS up and running, got models, and now started to code. Our code looks like a bunch of modules (both for Linux and for Windows) that need to be able to request files writing. So I decided not to play around with Inheritance and create a simple class that will grant each module an int value that will represent it's file access handle. For example, Display_Win will call GetFileAcc(filename), and it will get an int that it will then pass to the class. So, if it gets an error it could just do WriteToMyFile(int thatHandle, char *msg,...). But I realized that if I want to have scalability in my modules, I would need to have more than just 5 file handles. And I knew that a simple array would be too wasteful, so I embarked on making a linked list. After all, a linked list is perfect... I would always have only 5-15 file handles in a list, and I would need to remove file handles from the list handle quite often. But I am not quite sure I got it right. This crashes often for no reason at all :(. I think I made a mistake in actually creating the linked list, and not the file access. Can somebody please tell me what I did wrong?

/*

   writeLog.h
    HEADER

*/
#include <cstdio>
#include <cstdarg>
#include <cstdlib>

class writeLog{
public:    
        writeLog(); // Will initialize the begin node.
        ~writeLog();// Will free all memory
  
        int requestNode(char* filename);   // will make space and return a unique ID
        void killNode(int ID);             //Will  free that node.
        void record(int ID,char *msg,...); // Actual record thingy.   
private:
  struct fileNode{
         int ID; // Sigh, lets hope 2million should be enough.
         FILE *file; // IDs WILL _NOT_ BE IN ORDER!!!!!!!!
         fileNode *Next;
         }*begin;    
  
        int cID; // CURRENT ID number.
        fileNode* MakeNode(int ID,char *filename); 
        fileNode* NextNode(fileNode *Node); //return Node->Next
        fileNode* GetNode(int ID); //searches through array looking for Node->ID
        fileNode* GetLastNode();  //looks for last array piece
        fileNode* PrevNode(fileNode *Node); // gets Node-1
        void flushAll();
};




#include "writeLog.h"

#define null NULL



writeLog::writeLog()
{    cID=0;
     begin=(fileNode*) malloc( sizeof(fileNode));
     begin->ID=0; // ONLY 0
     begin->file=NULL;

}

int writeLog::requestNode(char *filename)
{
    cID++;
    
    fileNode* Node=MakeNode(cID,filename);
    fileNode* temp=GetLastNode();
    temp->Next=Node;
    
    return cID;
}

writeLog::fileNode* writeLog::MakeNode(int ID,char* filename)
{
  fileNode* Node = (fileNode*) malloc( sizeof(fileNode));          
  Node->ID=ID;
  Node->file=fopen(filename,"w");
  Node->Next=NULL;
  return Node;
}


void writeLog::record(int ID, char *msg,...)
{
 fileNode* Node = GetNode(ID);
   va_list args;
   va_start (args, msg);
   vfprintf (Node->file, msg, args);
   va_end (args);
   fflush(Node->file);
}

writeLog::fileNode* writeLog::GetLastNode()
{
 fileNode* Node = begin;
 for(;Node->Next!=NULL;Node=NextNode(Node) ); // Woosh. It works like a charm
  return Node;                                // ...I think.
}

writeLog::fileNode* writeLog::NextNode(fileNode *Node){
return Node->Next;}


writeLog::fileNode* writeLog::GetNode(int ID)
{ fileNode* temp;
  for(;temp->ID!=ID;temp==NextNode(temp));
  return temp;
}

void writeLog::killNode(int ID) // let the nasty stuff start
{
fileNode* temp = GetNode(ID);
fileNode* prev = PrevNode(temp);
fileNode* next = temp->Next;

fclose(temp->file);
free(temp);

prev->Next=next;
next->Next=NULL;
}

writeLog::fileNode* writeLog::PrevNode(fileNode* Node)
          {
          fileNode* prevNode=begin;
          fileNode* temp = begin;

          while(1)
                  {
                   if(temp==Node)
                   break;
                   prevNode=temp;
                   temp=NextNode(temp);
                  }
                return prevNode;
          }


writeLog::~writeLog()
{
  
  while(1)
  {   
  fileNode* Node = GetLastNode();
  if(Node==NULL)
  break;             
  fileNode* prev = PrevNode(Node);
  free(Node);
  prev->Next=NULL;
  }
}


Advertisement
There are a few problems with this. The first is that in "GetNode()" you don't initialize fileNode to point to "begin" before you start dereferencing it.

Also, in "MakeNode()" you never hook the new node you're creating into the list by assigning it to the Next* of some other node.

It looks like you aren't too familiar with linked lists - I would suggest reading up a bit more and familiarizing yourself with the concepts. Wikipedia is a good place to start.

Also, I like to draw them out on paper whenever I'm working with them. A series of boxes to represent the nodes and arrows to represent the pointers always seems to clear things up for me.
Quote:Original post by Cecil_PL
There are a few problems with this. The first is that in "GetNode()" you don't initialize fileNode to point to "begin" before you start dereferencing it.

Also, in "MakeNode()" you never hook the new node you're creating into the list by assigning it to the Next* of some other node.

It looks like you aren't too familiar with linked lists - I would suggest reading up a bit more and familiarizing yourself with the concepts. Wikipedia is a good place to start.

Also, I like to draw them out on paper whenever I'm working with them. A series of boxes to represent the nodes and arrows to represent the pointers always seems to clear things up for me.



"MakeNode" only gets called from "RequestNode" though... "RequestNode" assigns the node to the last node. So that shouldn't be the problem.
I am sorry, my naming scheme is a bit not clear. I should have named them better.

Thank you for the "GetNode" problem! Should this fix it?
//fileNode* temp;fileNode* temp = begin;


As for wikipedia, I will certainly look into it. Is my routine that broken?
Why are you writing your own linked list in the first place?
What's wrong with std::list, deque, vector or map?
A better solution is to make wider use of the C++ language.
I've just whipped this simple un-tested example up:

/*    logger.h*/#include <ostream>#include <map>#include <string>class Logger{public:    void addStream (const std::string& type, std::ostream& stream);    bool write (const std::string& type, const std::string&);private:    typedef std::multimap< std::string, OStreamList >  OStreamMap;    OStreamMap streams;};void Logger::addStream  (const std::string& type, std::ostream& stream){    streams.insert( OStreamMap::value_type( type, &stream ) );}bool Logger::write (const std::string& type, const std::string& message){    std::pair< OStreamMap::iterator, OStreamMap::iterator > streamRange;    streamRange = streams.equal_range( type );  // Find the list of streams to use    if( streamRange.first == streamRange.second ) {  // Ensure the range is valid        return false;    }    // Iterate each stream and output the message     for (OStreamList::iterator iter = streamRange.first;         iter != streamType->second();         ++iter)    {        (*iter->second) << message << std::endl;    }    return true;}


And then elsewhere, in main for example:

#include <fstream>#include <iostream>#include "logger.h"int main(){    Logger logger;    ofstream errorFile( "errors.txt" );    ofstream logFile ( "log.txt" );        logger.addStream( "logging", logFile );    logger.addStream( "logging", cout );    logger.addStream( "errors_only", errorFile );    logger.write( "logging", "This is a logged message!" );    logger.write( "errors_only", "This is an error message" );    return 0;}


Edit: Changed function signatures to use const std::string&

[Edited by - dmatter on August 14, 2007 11:21:55 AM]
Quote:Original post by Spoonbender
Why are you writing your own linked list in the first place?
What's wrong with std::list, deque, vector or map?

There is a standard list implementation?
As for deque, it won't work as I can't remove stuff from the middle :/.
Vector will be too slow to remove stuff from the middle.

Quote:
A better solution is to make wider use of the C++ language.
I've just whipped this simple un-tested example up: <snip>


Thanks! This looks cool! A mapped logger! :D
Quote:Original post by ShadowPhoenix
Quote:Original post by Spoonbender
Why are you writing your own linked list in the first place?
What's wrong with std::list, deque, vector or map?

There is a standard list implementation?
As for deque, it won't work as I can't remove stuff from the middle :/.
Vector will be too slow to remove stuff from the middle.

std::list is standard container implementing a doubly-linked list that offers efficient insertion and removal of elements to the front, back and middle of the container.
A deque can add and remove from the middle too just not as efficiently as a list, you cannot add elements to a deque without invalidating any outstanding iterators either.

Quote:
Quote:
A better solution is to make wider use of the C++ language.
I've just whipped this simple un-tested example up: <snip>


Thanks! This looks cool! A mapped logger! :D

It still remains un-tested, but you're welcome to use it [smile].
It's by no means the only way to create a logger, or the 'best' way either, its simply just one way of doing it.
Yes, there is a std::list (and as suggested above, std::map would probably be an even better fit)
Quote:Original post by ShadowPhoenix
Quote:Original post by Spoonbender
Why are you writing your own linked list in the first place?
What's wrong with std::list, deque, vector or map?

There is a standard list implementation?


Yep.

Quote:As for deque, it won't work as I can't remove stuff from the middle :/.


Nonsense. std::deque is identical to std::vector in this regard in that it offers linear time insertion and removal of elements from the middle.[1]
[TheUnbeliever]
Do you really need to store file handles?

Have you considered just opening files on demand?

Have you considered accumulating data in memory (likely with std::stringstream) to write all in one go?

This topic is closed to new replies.

Advertisement