views:

781

answers:

3

The jDeleteAfter method of my linked list class is supposed to delete the node immediately following the node passed as an argument. If it is doing that, I do not know, but it is abruptly closing my console application when "delete tlp;" (Temp List Pointer) gets read. My instructor, the users of a programming forum and I have yet to determine the root of this problem.

Written in Dev-C++ 4.9.9.2:

[source]
#include "JawaListT.h"
#include <cstdlib>
#include <iostream>
#include <new.h>

/*DEFAULT CONSTRUCTOR*/
JawaListT::JawaListT()
{
    if((this->jHead = new (nothrow) JawaLinkT) && (this->jTail = new (nothrow) JawaLinkT))
    {
     this->jHead->jSetNext(this->jTail);
     this->jTail->jSetNext(this->jTail);
    }//end if allocated
}

/*INSERT NODE AFTER*/
void JawaListT::jInsertAfter(JawaLinkT* lp, int val)
{
    if(lp != NULL && lp != this->jTail)  //if passed not tail and not null
    {
     JawaLinkT* tlp;    //new list node

     if((tlp = new (nothrow) JawaLinkT) != NULL) //if dynamically allocated 
     {
      tlp->jSetNext(lp->jGetNext()); //temp.next = passed.next     
      lp->jSetNext(tlp);  //passed.next = temp
      tlp->jSetValue(val);  //temp.data = val
     }//end if allocated
    }//end if not tail
}

/*INSERT NODE BEFORE*/
void JawaListT::jInsertBefore(JawaLinkT* lp, int val)
{
    if(lp != NULL && lp != this->jHead)  //if passed not head and not null
    {
     JawaLinkT* tlp;    //new list node

     if((tlp = new (nothrow) JawaLinkT) != NULL) //if dynamically allocated
     {
      tlp->jSetNext(lp->jGetNext());
      tlp->jSetValue(lp->jGetValue());
//    *tlp = *lp;   //copies passed node to temp node
      lp->jSetNext(tlp);  //passed.next = temp
      lp->jSetValue(val);  //passed.data = val
      if(lp == this->jTail)  //if passed is tail
      {
       this->jTail = tlp; //tail is temp
       this->jTail->jSetNext(this->jTail); //tail.next = tail
      }//end if lp
     }//end if tlp
    }//end if head
}

/*REMOVE NODE AFTER*/
void JawaListT::jDeleteAfter(JawaLinkT* lp)
{
    if(lp != NULL && lp->jGetNext() != this->jTail) //if not tail and not null
    {
     JawaLinkT* tlp;    //temp pointer to node

     tlp = lp->jGetNext();   //temp = passed.next
     lp->jSetNext(tlp->jGetNext());  //passed.next = temp.next
     delete tlp;    //delete to what temp points
    }//end if next 

     /*code that did not work any better*/
//   tlp->jSetNext((lp->jGetNext())->jGetNext()); 
//   delete lp->jGetNext();
//   lp->jSetNext(tlp);

/*Also tried declaring and/or deleting tlp outside of decision structure, and
jDeleteCurrent(tlp) since that function works properly.*/   
}

/*REMOVE CURRENT NODE*/
void JawaListT::jDeleteCurrent(JawaLinkT* lp)
{
    if(lp != NULL && lp != jHead && lp != jTail) //if not head or tail, not null
    { 
     JawaLinkT* tlp;    //temp pointer to node

     tlp = lp->jGetNext();   //temp = passed.next
     *lp = *tlp;    //copy temp to passed
     if(tlp == jTail)   //if temp is tail
     {
      this->jSetTail(lp);  //tail = passed
      lp->jSetNext(lp);  //passed.next = passed
     delete tlp;    //delete to what temp points
     }//end if tail
    }//end if not head
}

/*LINEAR SENTINEL SEARCH*/
JawaLinkT* JawaListT::jFindItemS(int item)
{
    JawaLinkT* tlp;     //temp pointer to node
this->jTail->jSetValue(item);      //tail.data = item

    for(tlp = jHead->jGetNext(); tlp->jGetValue() != item; tlp = tlp->jGetNext());
    /*INIT: node after head, EXIT: data found, UPDATE: increment node*/

    if(tlp == jTail)    //if sentinel found
      std::cout << item << " not in list" << std::endl; 

    return((tlp != this->jTail->jGetNext()) ? tlp : NULL);
    /*If sentinel not found, return proper node, else return null*/
}

[/source]

I use the class' sentinel search to traverse the list and provide the proper node as an argument for jDeleteAfter.

+2  A: 

A simple hint: remove all the tests for allocation failure - they will never happen on a Windows platform amd complicate the code. And if they do happen, you don't recover from them, so the tests are doubly useless.

anon
Neil: ignoring out of memory errors it bad practice. Don't teach this to newbees please.For a "show a code-snippet to the internet" it's a good thing to get rid of them though.
Nils Pipenbrinck
In C++, the best practice is to ignore it - an exception will be thrown. Disabling the exception and then not dealing with the error, as the questioner does, is the worst possible practice.
anon
As long as you're not on MSVC 6.0, which has a non-compliant operator new that returns NULL instead of throwing. (It might even be busted in 2003, I'm not sure.)
ephemient
A: 

Some code review tips:

JawaLinkT* tlp;                         //new list node

if((tlp = new (nothrow) JawaLinkT) != NULL)

is more readable as:

if(JawaLinkT* tlp = new (nothrow) JawaLinkT)

(also, see Neil's comment above why using nothrow without actually doing anything about it)

The code is also littered with random potential memory leaks:

if((this->jHead = new (nothrow) JawaLinkT) && (this->jTail = new (nothrow) JawaLinkT))
// What if first new succeeds and second fails?

As to the question, this is a lot of code to read without so much as stack trace to look for just a generic bug, but I think jDeleteAfter may be implemented incorrectly. Consider the case when the function is passed the node BEFORE the tail. I'll cut it off there because it does look like homework; but if you are still having trouble, comment and I'll clarify.

EDIT: And I realized I was wrong. Nevermind!

Todd Gardner
A: 

It turns out there was a conflict with my delete statement in my virtual destructor. It all works now. Thanks for the look-over of my code.

As for the nothrows -- I do it that way because our text introduced the idea and I don't know how to handle exceptions yet. Thanks for the advice, however.