1

This is my C++ code:

#include <iostream>

using namespace std;

typedef struct Node
{   
    int data;
    Node* next;
}Node;

class LinkedList
{
private:
    Node* first;
    Node* last;
public:
    LinkedList() {first = last = NULL;};
    LinkedList(int A[], int num);
    ~LinkedList();

    void Display();
    void Merge(LinkedList& b);
  
};

// Create Linked List using Array
LinkedList::LinkedList(int A[], int n)
{   
    Node* t = new Node;
    if (t == NULL)
    {
        cout << "Failed allocating memory!" << endl;
        exit(1);
    }
    t->data = A[0];
    t->next = NULL;
    first = last = t;

    for (int i = 1; i < n; i++)
    {
        t = new Node;
        if (t == NULL)
        {
            cout << "Failed allocating memory!" << endl;
            exit(1);
        }
        t->data = A[i];
        t->next = NULL;
        
        last->next = t;
        last = t;
    }
}

// Deleting all Node in Linked List
LinkedList::~LinkedList()
{
    Node* p = first;
    Node* tmp;

    while (p != NULL)
    {
        tmp = p;
        p = p->next;
        delete tmp;
    }
}

// Displaying Linked List
void LinkedList::Display()
{
    Node* tmp;

    for (tmp = first; tmp != NULL; tmp = tmp->next)
        cout << tmp->data << " ";
    cout << endl;    
}

// Merge two linked list
void LinkedList::Merge(LinkedList& b)
{
    // Store first pointer of Second Linked List
    Node* second = b.first;
    Node* third = NULL, *tmp = NULL;

    // We find first Node outside loop, smaller number, so Third pointer will store the first Node
    // Then, we can only use tmp pointer for repeating process inside While loop
    if (first->data < second->data)
    {
        third = tmp = first;
        first = first->next;
        tmp->next = NULL;
    }
    else
    {
        third = tmp = second;
        second = second->next;
        tmp->next = NULL;
    }

    // Use while loop for repeating process until First or Second hit NULL
    while (first != NULL && second != NULL)
    {
        // If first Node data is smaller than second Node data
        if (first->data < second->data)
        {
            tmp->next = first;
            tmp = first;
            first = first->next;
            tmp->next = NULL;
        }
        // If first Node data is greater than second Node data
        else
        {
            tmp->next = second;
            tmp = second;
            second = second->next;
            tmp->next = NULL;
        }
    }

    // Handle remaining Node that hasn't pointed by Last after while loop
    if (first != NULL)
        tmp->next = first;
    else
        tmp->next = second;

    // Change first to what Third pointing at, which is First Node
    first = third;    

    // Change last pointer from old first linked list to new last Node, after Merge
    Node* p = first;
    while (p->next != NULL)
    {
        p = p->next;
    }    
    last = p;
    
    // Destroy second linked list because every Node it's now connect with first linked list
    // This also prevent from Double free()
    b.last = NULL;
    b.first = NULL;
}

int main()
{
    int arr1[] = {4, 8, 12, 14, 15, 20, 26, 28, 30};
    int arr2[] = {2, 6, 10, 16, 18, 22, 24};
    int size1 = sizeof(arr1) / sizeof(arr1[0]);
    int size2 = sizeof(arr2) / sizeof(arr2[0]);
    
    LinkedList l1(arr1, size1);
    LinkedList l2(arr2, size2);

    l1.Display();
    l2.Display();
    
    // Merge two linked list, pass l2 as reference
    l1.Merge(l2);
    l1.Display();

    return 0;
}

I'm beginner on C++ and in this code, I practice how to Merge two linked list. This actually works perfectly. I've successfully Merge the two Linked List in sorted order.

But, there's people said that I should've follow the Rule of Three on C++. Which implement: Destructor, Copy Constructor, and Copy Assignment Operator.

I've watched many videos about that. I do understand that is basically handle Shallow Copy especially when we don't want two different object point to the same address of memory. But, for my problem is, I still don't know how to Implement it on a Class that working on a Linked List just like my code above.

Someone said, in my main(), this code: l1.Merge(l2); is somehow incorrect because I don't have explicit Copy Constructor.

And if you look at my Merge() function, in Last line, if I didn't to this: b.last = NULL; and b.first = NULL; , which simply destroy pointer of Second Linked list, the Compiler give me warning: Double free() detected.

So, I think my question is:

  1. How can this code: l1.Merge(l2); is have something to do with Copy Constructor?
  2. Is Double free() happened because I don't implement the Rule of Three? If yes, how to address them?
  3. How to write the Rule of Three based on my code? When or How to use them?
  4. Based on this Code, is there something wrong? Do I still need the Rule of Three if my Program only want to Merge Linked List?

Thank You. I hope someone can explain to me like I'm 10 years old. and hope someone can write me some Code.

Kevinkun
  • 21
  • 5
  • ***How to write the Rule of Three based on my code? When or How to use them?*** Any time your class allocates memory that it owns and uses raw pointers you will need to follow the rule of 3 or 5. – drescherjm Jul 16 '21 at 16:56
  • ***Is Double free() happened because I don't implement the Rule of Three?*** Yes a double free is a likely outcome of not implementing the rule of 3. – drescherjm Jul 16 '21 at 16:57
  • @drescherjm Can you be more specific? Is my **default constructor** not enough to handle it? – Kevinkun Jul 16 '21 at 17:00
  • 1
    @Kevinkun `l1 = l2;` -- and -- `LinkedList l3 = l1;` -- Did you try that simple test? Your `main` program seems to avoid having to actually test copying and assignment. You will see things fall apart or work correctly by simply writing those test lines. – PaulMcKenzie Jul 16 '21 at 17:04
  • @Kevinkun BTW, you should have implemented the rule of 3 way in advance of writing `Merge` or any such function. Maybe insert and delete, but anything more complex than that should have been put on hold *after* you've confirmed that copying, assignment, and deletion (the rule of 3) is working properly. Right now, you are adding functions on top of a faulty foundation. In your case, right after you implemented the constructor to insert the array items into the list, the very next thing would be to implement copy and assignment (and destruction). – PaulMcKenzie Jul 16 '21 at 17:12
  • 1
    A possible implementation is to just explicitely *delete* the copy ctor and assignment operator if you do not need them. That way if your code later tries to use them, you will get a compile time error. And as you *destroy* `l2` in `merge`, I would have used a `&&` reference to make it explicit. – Serge Ballesta Jul 16 '21 at 17:14
  • 1
    You have to implement the destructor. You don't have to implement the other two, you can opt to make them [deleted](https://en.cppreference.com/w/cpp/language/function#Deleted_functions) instead, in which case your linked lists will be non-copyable (and the compiler will yell at you if you accidentally try to copy them). – n. m. could be an AI Jul 16 '21 at 17:21
  • @SergeBallesta Can you give some explanation why I should use &&? and how to use it? – Kevinkun Jul 16 '21 at 17:51
  • https://stackoverflow.com/questions/12606574/understanding-rvalue-references – super Jul 16 '21 at 18:11

2 Answers2

0

There are several questionable practices applied in this code, and there is also a bug.

First, the bug. When you create a list, it news all its nodes and keeps track of them using pointers. When you assign a list to another, you literally copy the pointer values. Not only have you now lost the nodes of the assigned list (because you overwrote them) and got a memory leak (because now there's no pointer pointing to the allocated nodes), you also now have the same pointers on two different lists, pointing to the same nodes. When the lists are destroyed, both of them try to delete their nodes, and you end up freeing the same memory twice. Yuk.

The solution to this bug is to implement the assignment operator.

Then, the questionable practices:

  1. using namespace std; (Why is "using namespace std;" considered bad practice?)
  2. You're assigning the members of LinkedList in the constructor body, instead of passing the values directly to their constructor in the initialization list. (What is this weird colon-member (" : ") syntax in the constructor?)
  3. Declaring an array parameter (int[]) is declaring a pointer. Just be aware of it.
  4. new cannot return NULL! It's useless to check its return value. If it can't allocate, it will simply throw an exception.
  5. NULL is the inappropriate constant to use. You can use nullptr, it's the C++ equivalent of NULL, except it's type safe.
  6. Manual memory management with new and delete is tricky to get right (as you figured out yourself). You might be interested in using std::unique_ptr or std::shared_ptr to alleviate the burden. They would have caught the bug.

Now, please: do not write in C++ like it's C with classes. I understand that you may not have encountered all of the features I presented here, but anyway now you know about them :)

super
  • 278
  • 1
  • 11
  • Thank's for you questionable practices! I understand with your explanation about memory leaks. But, which create code is bug? Do you mean this create code is wrong: `LinkedList l1(arr1, size1);` and `LinkedList l2(arr2, size2);` ? and which code exactly I have to use it with the assignment operator? – Kevinkun Jul 16 '21 at 18:23
  • Because the code it's actually works perfectly. I handle the memory leaks by **Pass by reference** and destroy `l2` by assigning `b.first = NULL` and `b.last=NULL` so when destructor call, it will not **double free**. My problem is, I want to know if maybe there's a good practice on my code by using the Rule of Three? – Kevinkun Jul 16 '21 at 18:27
  • *For comment 1*: The bug I was talking about happens when you assign `LinkedList`. Even though you haven't defined an assignment operator, the compiler defined one for you that just so happens to do the wrong thing. (Note: you don't use the assignment operator in the code you posted. That's why some comments suggested marking it as `delete`d (something totally different from memory deallocation btw) so it will never get called. The bug however is still there, it's just not presenting itself). – super Jul 16 '21 at 18:34
  • *For comment 2*: Using the rule of three is generally good practice when you can't follow the rule of zero. That is, when the compiler provided constructors and assignment operators won't do what you want. The bottom line here is the less code you write, the better, so let the compiler write it for you if you can. **Edited:** answer was partly wrong because I thought you were talking about some other part of the code. – super Jul 16 '21 at 18:36
  • Also, here's a tip: you can call the constructor for `Node`, too, when you `new` it. The compiler generated one for you that does what you want in that case (look up aggregate initialization). – super Jul 16 '21 at 18:42
  • @Kevinkun *My problem is, I want to know if maybe there's a good practice on my code by using the Rule of Three?* -- I guess you didn't see my comment in the main section. A simple assignment, and your linked list falls apart. The only way to prevent it is to disallow copying and assignment at compile-time. Also, your `main` program you posted did not exercise your code enough to discover these bugs -- it is basically biased towards not finding the issues, and that is not how you're supposed to test these issues. – PaulMcKenzie Jul 16 '21 at 18:44
  • @PaulMcKenzie I saw your comment. So, if in my `main` program doesn't have any code that perform copying or assignment just like you said, I don't need to implement the rule of three? or, _based on my code_, there's some part I can implement with the rule of three? – Kevinkun Jul 16 '21 at 18:54
  • Sure. If you don't shoot yourself, it's fine to keep a gun pointed ad your foot. But you should avoid doing that. – super Jul 16 '21 at 18:56
  • You need to enforce this at compile-time by explicitly declaring the copy constructor and assignment operator with the `= delete` specifier. If you don't do that, then your code will easily fall apart at places you never expect. For example, if you were to place your `LinkedList` class in a container that does copies behind the scenes (such as `std::vector`), then you're in trouble. If you pass or return by value, you're in trouble. If the compiler itself needs to make a copy without you even suspecting it, you're in trouble. So you either make the class copyable, or turn copying off. – PaulMcKenzie Jul 16 '21 at 19:01
  • Do not use `unique_ptr` or `shared_ptr` to implement a linked list, unless you like stack overflows. – n. m. could be an AI Jul 16 '21 at 19:06
  • @PaulMcKenzie Okay. But, last question, People said I have to follow the Rule of Three for my code above. The thing is, I don't understand which code that I've wrong implemented it. Because, all I know is the code works perfectly. So, if there's any, maybe you can point out which part of my code that I should implementing it using copy constructor or assignment operator? – Kevinkun Jul 16 '21 at 19:14
  • @Kevinkun -- What if *I* decide to use your linked list class? You have no idea how I will use it. It needs to be safely copyable and assignable, otherwise it is basically worthless. Again, your `main` program is biased towards *not* showing the errors. Anyone can write buggy classes, and write code that doesn't show the errors that are lurking. It's as soon as someone independent of the original programmer starts to use the class, then all the bugs come to light. – PaulMcKenzie Jul 16 '21 at 19:15
  • I already did write the code: `l1 = l2;`. Just that alone shows the errors. Or `LinkedList l3 = l1;` or `std::vector vL; vL.push_back(l1);` – PaulMcKenzie Jul 16 '21 at 19:19
0

But, for my problem is, I still don't know how to Implement [Rule of Three] on a Class that working on a Linked List just like my code above.

You simply implement the copy constructor and copy assignment operator to iterate the input list, making a copy of each node and inserting them into your target list. You already have a working destructor. In the case of the copy assignment operator, you can usually use the copy-swap idiom to implement it using the copy constructor to avoid repeating yourself.

Someone said, in my main(), this code: l1.Merge(l2); is somehow incorrect because I don't have explicit Copy Constructor.

Then you were told wrong. Your Merge() code has nothing to do with a copy constructor.

And if you look at my Merge() function, in Last line, if I didn't to this: b.last = NULL; and b.first = NULL;, which simply destroy pointer of Second Linked list, the Compiler give me warning: Double free() detected.

Correct. Since you are moving the nodes from the input list to the target list, you need to reset the input list so it doesn't point at the moved nodes anymore. Otherwise, the destructor of the input list will try to free them, as will the destructor of the target list.

How can this code: l1.Merge(l2); is have something to do with Copy Constructor?

It doesn't have anything to do with it.

Is Double free() happened because I don't implement the Rule of Three?

Not in your particular example, as you are not performing any copy operations. But, in general, not implementing the Rule of Three can lead to double frees, yes.

How to write the Rule of Three based on my code?

See the code below.

Do I still need the Rule of Three if my Program only want to Merge Linked List?

No. Only when you want to make copies of lists.

With that said, here is an implementation that includes the Rule of Three:

#include <iostream>
#include <utility>

struct Node
{
    int data;
    Node *next;
};

class LinkedList
{
private:
    Node *first;
    Node *last;
public:
    LinkedList();
    LinkedList(const LinkedList &src);
    LinkedList(int A[], int num);
    ~LinkedList();

    LinkedList& operator=(const LinkedList &rhs);

    void Display() const;
    void Merge(LinkedList &b);
};

// Create Linked List using default values
LinkedList::LinkedList()
    : first(NULL), last(NULL)
{
}

// Create Linked List using Array
LinkedList::LinkedList(int A[], int n)
    : first(NULL), last(NULL)
{
    Node **p = &first;

    for (int i = 0; i < n; ++i)
    {
        Node *t = new Node;
        t->data = A[i];
        t->next = NULL;

        *p = t;
        p = &(t->next);

        last = t;
    }
}

// Create Linked List by copying another Linked List
LinkedList::LinkedList(const LinkedList &src)
    : first(NULL), last(NULL)
{
    Node **p = &first;

    for (Node *tmp = src.first; tmp; tmp = tmp->next)
    {
        Node* t = new Node;
        t->data = tmp->data;
        t->next = NULL;

        *p = t;
        p = &(t->next);

        last = t;
    }
}

// Deleting all Node in Linked List
LinkedList::~LinkedList()
{
    Node *p = first;

    while (p)
    {
        Node *tmp = p;
        p = p->next;
        delete tmp;
    }
}

// Update Linked List by copying another Linked List
LinkedList& LinkedList::operator=(const LinkedList &rhs)
{
    if (&rhs != this)
    {
        LinkedList tmp(rhs);
        std::swap(tmp.first, first);
        std::swap(tmp.last, last);
    }
    return *this;
}

// Displaying Linked List
void LinkedList::Display() const
{
    for (Node *tmp = first; tmp; tmp = tmp->next)
        std::cout << tmp->data << " ";
    std::cout << std::endl;
}

// Merge two linked list
void LinkedList::Merge(LinkedList& b)
{
    if ((&b == this) || (!b.first))
        return;

    if (!first)
    {
        first = b.first; b.first = NULL;
        last = b.last; b.last = NULL;
        return;
    }

    // Store first pointer of Second Linked List
    Node *second = b.first;
    Node *third, **tmp = &third;

    // We find first Node outside loop, smaller number, so Third pointer will store the first Node
    // Then, we can only use tmp pointer for repeating process inside While loop
    // Use while loop for repeating process until First or Second hit NULL
    do
    {
        // If first Node data is smaller than second Node data
        if (first->data < second->data)
        {
            *tmp = first;
            tmp = &(first->next);
            first = first->next;
        }
        // If first Node data is greater than second Node data
        else
        {
            *tmp = second;
            tmp = &(second->next);
            second = second->next;
        }
        *tmp = NULL;
    }
    while (first && second);

    // Handle remaining Node that hasn't pointed by Last after while loop
    *tmp = (first) ? first : second;

    // Change first to what Third pointing at, which is First Node
    first = third;  

    // Change last pointer from old first linked list to new last Node, after Merge
    Node *p = first;
    while (p->next)
    {
        p = p->next;
    }   
    last = p;
    
    // Destroy second linked list because every Node it's now connect with first linked list
    // This also prevent from Double free()
    b.first = b.last = NULL;
}

int main()
{
    int arr1[] = {4, 8, 12, 14, 15, 20, 26, 28, 30};
    int arr2[] = {2, 6, 10, 16, 18, 22, 24};
    int size1 = sizeof(arr1) / sizeof(arr1[0]);
    int size2 = sizeof(arr2) / sizeof(arr2[0]);
    
    LinkedList l1(arr1, size1);
    LinkedList l2(arr2, size2);
    LinkedList l3(l1);
    LinkedList l4;

    l1.Display();
    l2.Display();
    l3.Display();
    l4.Display();
    
    // Merge two linked list, pass l2 as reference
    l3.Merge(l2);
    l4 = l3;

    l1.Display();
    l2.Display();
    l3.Display();
    l4.Display();

    return 0;
}

Demo

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • Wow. Thank's for your explanation! – Kevinkun Jul 16 '21 at 19:29
  • So, for this function: `LinkedList::LinkedList(const LinkedList &src)` and `LinkedList& LinkedList::operator=(const LinkedList &rhs)` this will only execute if I perform Copying from existing object to new object on my code? Like `l1 = l2` or `LinkedList l2(l1)` ? – Kevinkun Jul 16 '21 at 19:32
  • Yes, those are the copy constructor and copy assignment operator, they are invoked only when making copies. And if you really want to be thorough, consider the Rule of Five, which adds a move constructor and move assignment operator, to *steal* resources without making copies. Move semantics were introduced in C++11 – Remy Lebeau Jul 16 '21 at 19:37