C ++ implementation of pointers / lists

I think you are done developing the list node modeling. The ListNode class IS A list node, as shown by its name. In this case, no nested structure is required to store the list, which is very confusing.

More detailed feedback is at the bottom of this post, but just some inline comments and code changes to get started:

struct Node // Why doesn't this have a constructor initializing the members?
{
        int value;
        Node *next;
} *lnFirst; 


ListNode::ListNode() : lnFirst(NULL) {} // Use initializer lists instead of initializing members in the ctor body. It cleaner, more efficient and may avoid some nasty bugs (because otherwise the member gets default-initialized *first*, and then assigned to in the body)

int ListNode::Length()
{
    int intCount = 0;
    for( Node* lnTemp=lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next ) // Create the loop iteration variable lnTemp here, in the loop, not at the start of the function
    {
        intCount++;
    }
    return intCount;
}

void ListNode::DisplayList()
{
    for(Node* lnTemp = lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next ) // And again, initialize the loop variable in the loop
        cout<<endl<<lnTemp->value; // Not a huge deal, but endl flushes the stream as well as inserting a newline. That can be needlessly slow. So you might want to just use "\n" in cases where you don't need the flushing behavior.
}

void ListNode::Insert(int num)
{
//    Node *lnCurrent, *lnNew; // Very subjective, but I prefer not declaring multiple variables on the same line, because the syntax if they're pointers can be surprising (You got it right, but a lot of people would write Node* lnCurrent, lnView, which would make lnView not a pointer. I find it clearer to just give ecah variable a separate line:
    if( lnFirst == NULL )
    {
//        lnFirst = new Node;
//        lnFirst->value = num;
//        lnFirst->next = NULL;
        lnFirst = new Node(num); // Make a constructor which initializes next to NULL, and sets value = num. Just like you would in other languages. ;)
    }
    else
    {
        Node* lnCurrent = lnFirst; // Don't declare variables until you need them. Both to improve readability, and because C++ distinguishes between initialization and assignment, so in some cases, default-initialization followed by assigment may not be the same as just initializing with the desired value.
        while( lnCurrent->next != NULL )
                lnCurrent = lnCurrent->next;

        Node* lnNew = new Node(num); // Again, let a constructor do the work.
        lnCurrent->next = lnNew;
    }
}

bool ListNode::Contains(int num)
{
    bool boolDoesContain = false;
//    Node *lnTemp,*lnCurrent; // Again, don't initialize variables at the start of the function if they're not needed
    Node* lnCurrent = lnFirst;
//    lnTemp = lnCurrent;
    while( lnCurrent!=NULL )
    {
        if( lnCurrent->value == num )
        {
//                boolDoesContain = true;
//                return boolDoesContain;
                  return true; // Just return directly, and remove the boolDoesContain variable. Alternatively, set boolDoesContain to true, and then break out of the loop without returning, so you have a single exit point from the function. Both approaches have their merits, but setting a variable you don't need, and then returning is silly. ;)
        }
//        Node* lnTemp = lnCurrent; // you don't actually use lnTemp for anything, it seems
        lnCurrent = lnCurrent->next;
    }
//    return boolDoesContain;
      return false; // just return false. If you get this far, it must be because you haven't found a match, so boolDoesContain can only be false anyway.
}

int ListNode::GetValue(int num)
{
//    Node *lnTemp;
    int intCount = 1; // Wouldn't most people expect this indexing to be zero-based?
    for( Node* lnTemp=lnFirst; lnTemp != NULL; lnTemp = lnTemp->next )
    {
        if (intCount == num)
        {
            return lnTemp->value;
        }
        intCount++;
    }    
}

      

        
        
        
      

    

Now a few general remarks. (I'm going to ignore if you misunderstood the assignment and focus on the code you posted) First, Hungarian notation: Don't. First call pointers node, temp and everything else, without the 'ln' prefix. Call your doContain bool variable without the extra "bool" prefix. Second, as I tried to do in the edited code, create variables only when you need them. C is used to request variables to be declared at the top of a block, but C ++ never did. Third, you don't need a "return 0" at the end of the main function. Main is a special case where if it reaches the end of the function it will automatically return 0.

Fourth, we have a big annoying problem: memory management. You are allocating memory that is never freed. Since you don't have a RemoveNode function, this might sound moot, but what happens when the entire list goes out of scope and gets removed by itself? None of its nodes are deleted because the entire list has a bunch of pointers and it doesn't automatically trigger deletion on them. Therefore, at least you need a destructor in the list class itself, so that when the list is deleted, it will definitely delete all of its child nodes.

This should handle the simple default case where you create a list, add nodes to it, and remove the list.

The next big problem is, what if I copy the list?

int main(){
ListNode list;
list.Insert(1);
list.Insert(2);
list.Insert(3);
}
ListNode list2 = list;

      

        
        
        
      

    

Your code explodes. Both lists now point to the same nodes, instead of making copies of the nodes. Adding a node to one list will also make it appear in another. And before you state "that's a function, not a bug";), think about what happens when one of the lists is deleted.

Suppose list2 is removed first. With the destructor we just defined, it removes three nodes and returns. Now we list the nodes that have been removed. Accessing them is undefined behavior and will most likely fail. So let's say we're not referring to them, instead we'll just just delete that list.

This means that we are trying to delete child nodes that have already been deleted. It definitely sounds like an accident.

So, to fix this, the ListNode class has to implement two additional functions: a copy constructor and an assignment operator:

ListNode(const ListNode& other);
ListNode& operator==(const ListNode& other);

      

        
        
        
      

    

These two should ensure that when all data is copied from the "other". For each node in the "other", you must allocate a new node in the current list, not just make both lists pointers to the same node. (This means the node class most likely needs a copy constructor too, at least).

This is the main way to manage memory and it's important to understand because otherwise you will mess up.;)

Part of the assignment:

Provide the following features:

• ListNode (int v, ListNode * l)
• int getValue ();
• ListNode * getNext ();
• void insert (int i);
• bool listcontains (int j);

You have not provided any of these features.

Since some others have a pointer, you have implemented a List instead of a ListNode, so your function signatures are different.

But you also shouldn't mindlessly violate the job coding rules. Do you have a C # -background? C ++ coding conventions usually specify lowercase naming conventions.