traversing to delete a node issue

[kyungturner]

i get a segment fault when i try to delete it. i know that head is null and because of it the loop condition wont execute. i just dont know how to fix it!

#include <iostream>
#include <cstring>
using namespace std;

//model for node, gives node what to store
struct node
{
  char title [51];
  int date;
  char rate[6];
  char temp[100];
  char data;
  char * comment;  
  node * next;
};

//start prototypes
void welcome(node *& head, node * current, node * previous);
void add(node * &head, node * current, node * previous);
void display(node * head, node * current, node * previous);
void del(node * & head, node * current, node * previous);

int main()
{
  node * head = NULL;
  node * current = NULL;
  node * previous = NULL;

  welcome(head,current,previous);
   return 0;
}

void del(node * &head, node * current, node * previous)
{
  char * option;

  cout<<"\t\tPlease type in a key(WORD) to "<<endl;
  cout<<"\t\tmatch your search to delete: ";
  cin>>option;
  cin.ignore(100,'\n');
  cout << head->title << endl;
    current = head; 
    previous = head;
    while (current && strcmp (current->title, option) != 0) 
    {
      cout<<current->title<<endl;
      previous = current;
      current = current->next;

    }     
    if (current != NULL)
    {
      previous->next = current->next;
      delete current;
    }
  }



void welcome(node * & head, node * current, node * previous)
{
  char option;
  cout<<"\n\t\tWelcome to the Portland Bicycle review!";
  cout<<"\n\n\tWould you like to Write a (r)eview, (t)ake out reviews or (d)isplay reviews?: ";
  cin>>option;
  cin.ignore(100,'\n');
  cout<<endl;

  if (option == 'r')
  {
    do
    {
      
      add(head,current,previous);

      cout<<endl<<"\t\tAdd more? ";
      cout<<endl<<"\t\tEnter (y)es or (n)o: "; 
      cin>>option;
      cin.ignore(100,'\n');
      if (option == 'n')
	welcome(head,current,previous);
      else
	option = true;
    }
    while (option);
  }
  else if (option == 'd')
  {
    display(head,current,previous);
    welcome(head,current,previous);
     
  }
  else
    del(head,current,previous);
}

void display(node * head, node * current, node * previous)
{
    current = head;
    while (current != NULL)
    {
    cout<<endl<<"\t\tTitle: "<<current->title<<endl;
    cout<<"\t\tDate (MMDD): "<<current->date<<endl;
    cout<<"\t\tSafety Rating: "<<current->rate<<endl;
    cout<<"\t\tComment: "<<current->comment<<endl;

    current = current -> next;
    }
    return;
}

void add(node *& head, node * current, node * previous)
{
  char option;
  if (!head)
  {
    head = new node;
      cout<<"\n\t\tEnter a title: ";
      cin.get(head->title,51,'\n');        
      cin.ignore(100,'\n');
      cout<<"\t\tEnter a date mmdd (0601): ";
      cin>>head->date;
      cin.ignore(100,'\n'); 
      cout<<"\t\tEnter a safety rating * through *****: ";
      cin>>head->rate;
      cin.ignore(100,'\n');  
      cout<<"\t\tEnter a short comment: ";
      cin.get(head->temp,100,'\n');
      head->comment = new char [strlen(head->temp)+1];
      strcpy(head->comment,head->temp);
      cin.ignore(100,'\n');
    current = head;
    head->next = NULL;
    return;
  }
  else
  {
    
      current = new node;   
      cout<<"\n\t\tEnter a title: ";
      cin.get(current->title,51,'\n');        
      cin.ignore(100,'\n');
      cout<<"\t\tEnter a date mmdd (0601): ";
      cin>>current->date;
      cin.ignore(100,'\n'); 
      cout<<"\t\tEnter a SAFETY rating * through *****: ";
      cin>>current->rate;
      cin.ignore(100,'\n');  
      cout<<"\t\tEnter a short comment: ";
      cin.get(current->temp,100,'\n');
      current->comment = new char [strlen(current->temp)+1];
      strcpy(current->comment,current->temp);
      cin.ignore(100,'\n');
      current->next = head;
      head = current;
        

  }
}

[zb0S216C]

Well.... what line? Give us some direction.

_Wazzak

[kyungturner]

between the lines 33 throu 56, on void del();

thanks!

[zb0S216C]

Call the plumber, we got Niagara Falls over here. Your code is about as leak-safe as a colander. I see multiple calls to new[] but a deficiency of calls to delete[]. Not only that, you're overwriting allocate memory which is causing even more leaks.

I don't even know what I'm looking for, nor do I know where to begin to look. Here's what I suggest you do:

- Ensure proper deallocation of accumulated resources
- Ensure at least 1 pointer has ownership of accumulated resources
- You're using C++ - make use of classes so that proper memory management is performed
- Comment your code more. You may know what your code does now, but in the future, you may not

_Wazzak

[kyungturner]

would i delete simply at the end of the main? thanks again!

delete [] node;

[zb0S216C]

It's not as simple as that. Here's a bad example of memory management:

void ALeakingFunction()
{
    int *Memory(new int[3]);
}

Here, only Memory knows where the accumulated 3 ints are. When Memory is destroyed, the memory once pointed to is now lost. Thus, a leak of accumulated resources. The ideal solution here is to releases the resources prior to leaving the function:

void ALeakingFunctionNoMore()
{
    int *Memory(new int[3]);
    delete [] Memory;
}

Classes are useful for exactly this reason. It's common to see "Smart Pointers". Theses are classes that exploit the constructor/destructor functionality. Here's a home-made one:

template <typename Type>
class SafePointer
{
    public:
        SafePointer(Type * const NewResource = NULL)
            : Resource__(NewResource)
        { }

       ~SafePointer()
        {
            delete this->Resource__;
        }

        Type &operator * () { return(*this->Resource__); }
        Type const &operator * () const { return(*this->Resource__); }
        Type *operator -> () { return(this->Resource__); }
        Type const *operator -> () const { return(this->Resource__); }

    private:
        Type *Resource__;
};

template <typename Type>
class SafeArray
{
    public:
        SafeArray(Type * const NewResource = NULL)
            : Resource__(NewResource)
        { }

       ~SafeArray()
        {
            delete [] this->Resource__;
        }

        Type &operator [] (unsigned int const &Index)
        { return(this->Resource__[Index]); }

        Type const &operator [] (unsigned int const &Index) const
        { return(this->Resource__[Index]); }

    private:
        Type *Resource__;
};

Here's them both in action:

int main()
{
    {
        SafePointer<int> MySafePointer(new int(100));
    }

    {
        SafeArray<int> MySafeArray(new int[3]);
    }
}

Both are safe. It's guaranteed that the accumulated memory above will be released when both leave their respective scopes. So if you forget the release the memory, the destructor will do it for you. Feel free to use these classes :)

Note: Untested code.

_Wazzak

[BlackSheep]

@Framework:
Couldn't this cause problems if your safe pointer is destroyed at its end-of-scope, but the memory still needs to be accessed by another object?

[zb0S216C]

Yes, it could, since it lacks a reference counter. However, I wrote the class so that it only handles proper de-allocation, not reference counting nor ownership management. The class can easily be changed to support the said features.

_Wazzak