2
\$\begingroup\$

This is the link list I just created. It works fine for now, but would want to know if I could make it better.

list.hpp

/******************************************************************************************************
 * Purpose: Custom Doubly Linked-list.
 *
 * Author: Sahil Arora
 *
 * Version : 1.0
 *
 * Date of Creation : 06/04/2016
 *
 * Usage: Include "list.hpp" and create objects.
 *
 * Bugs : None
******************************************************************************************************/
#ifndef LIST_HPP_
#define LIST_HPP_

namespace mod
{
    template<typename T>
    struct link{
            T value;
            link<T>* prev;
            link<T>* next;
    };

    template<typename T>
    class list
    {
        int len;
        link<T> *head, *tail;
        public:
            list() {head = tail = nullptr; len = 0;}    //default constructor
            list(list<T>& x);                           //copy constructor
            ~list();                                    //destructor
            void append(const T& value);                //appending an element
            void append(list<T>& x);                    //appending a list
            int length() {return len;}                  //returning length of the list
            bool empty() {return len<=0;}               //check if empty
            void cons(const T& value);                  //adding an element to the head
            void remove(const T& x);                    //remove the first occurence of an element
            link<T>* get_head() { return head; }        //return head pointer
            link<T>* get_tail() { return tail; }        //return tail pointer
            class iterator{                             //iterator class
                link<T> *ptr;
                public:
                    iterator() {ptr = nullptr;}         
                    iterator(link<T>* p) {ptr = p;}
                    link<T>* get_ptr(){return ptr;}
                    T& operator*(){return ptr->value;}
                    void operator=(iterator iter){ptr = iter.get_ptr();}
                    bool operator==(iterator iter){return ptr == iter.get_ptr();}
                    void operator++() { if(ptr!=nullptr) ptr = ptr->next; }
                    void operator--(){ if(ptr!=nullptr) ptr = ptr->prev;}
                    bool operator!=(iterator iter){return ptr != iter.get_ptr();}  
            };
            iterator begin() {return iterator(head);}   //begin iterator
            iterator end() {return iterator(nullptr);}  //end iterator
    };

    template<typename T>
    list<T>::list(list<T>& x){
        link<T>* ptr = x.get_head();
        if(x.length()==0){
            head = tail = nullptr;
            return;
        }
        link<T>* ptr2 = new link<T>;
        ptr2->value = ptr->value;
        ptr2->prev = ptr2->next = nullptr;
        head = ptr2; 
        ptr = ptr->next;
        while(ptr!=nullptr){
            link<T>* temp = new link<T>;
            temp->value = ptr->value;
            temp->next = nullptr;
            temp->prev = ptr2;
            ptr2->next = temp;
            ptr2 = temp;
            ptr = ptr->next;
        }
        tail = ptr2;
        len = x.length();
    }

    template<typename T>
    list<T>::~list(){
        link<T> *ptr = head; 
        while(ptr != nullptr){
            link<T>* ptr2 = ptr->next;
            delete ptr;
            ptr = ptr2;
        }
    }

    template<typename T>
    void list<T>::append(const T& a){
        link<T> *ptr = new link<T>;
        ptr->value = a;
        ptr->next = nullptr;
        ptr->prev = tail;
        if(len==0){
            head = tail = ptr;
            ++len;
            return;
        }
        tail->next = ptr;
        tail = ptr;
        ++len;
    }

    template<typename T>
    void list<T>::cons(const T& a){
        link<T> *ptr = new link<T>;
        ptr->value = a;
        ptr->next = head;
        ptr->prev = nullptr;
        head->prev = ptr;
        head = ptr;
        ++len;
    }

    template<typename T>
    void list<T>::remove(const T& a){
        link<T> *ptr = head;
        if(len>0 && head->value == a){
            head = head->next;
            if(len>1)
                head->prev = nullptr;
            delete ptr;
            --len;
            return;
        }
        while(ptr!=nullptr){
            if(ptr->value == a){
                ptr->prev->next = ptr->next;
                if(ptr==tail)
                    tail = ptr->prev;
                else
                    ptr->next->prev = ptr->prev;
                delete ptr;
                --len;
                return;
            }
            ptr = ptr->next;
        }
    }

    template<typename T>
    void list<T>::append(list<T>& x){
        for(list<T>::iterator it = x.begin() ; it!=x.end(); ++it)
        append(*it);
    }

}

#endif

tester.cpp

Tester program:

#include "list.hpp"
#include <iostream>

 using namespace mod;
 using std::cout;
 using std::cin;
 template<typename T>
 void print(list<T>& l){
    cout<<"List Length: "<<l.length()<<"\nList: ";
    for(list<int>::iterator it = l.begin() ; it!=l.end(); ++it)
        cout<<*it<<" ";
    cout<<"\n";
 }
int main(int argc, char const *argv[])
{
    list<int> l1;
    print(l1);
    for(int i=0;i<10;++i)
        l1.append(i*10);
    print(l1);
    list<int> l2;
    for(int i=0;i<10;++i)
        l2.append(i*15);
    print(l2);
    l2.append(l1);
    print(l2);
    list<int> l3(l2);
    print(l3);
    for(int i=0;i<10;++i)
        l3.append(i*3);
    print(l3);
    l3.append(l1);
    l3.remove(10);
    l3.remove(20);
    l3.remove(30);
    l3.remove(40);
    l3.cons(-155);
    print(l3);
    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ your append(&list) is broken; it'll double free the nodes that were in the passed in list. So either duplicate the list or clear the passed in one (and change the name) \$\endgroup\$ – ratchet freak Apr 6 '16 at 10:49
  • \$\begingroup\$ Yeah! This was a great advice. I'll do that. \$\endgroup\$ – Sahil Arora Apr 6 '16 at 11:27
2
\$\begingroup\$

Code readabilty: if you decide to implement the member functions outside the class definition, do that for all of them. Including constructors and destructor.

Use c++ syntax to initialize your data members before the constructor body for example:

list () : len(0), head(nullptr), tail(nullptr) {}

Don't create constructors (especially copy-constructors) if you don't **absolutely ** have to, use default ones when you can.

For example: your iterator class.

A good rule of thumb is: If you feel you must explicitly declare one of the three:

  • copy constructor

  • assignment

  • destructor

You must declare them all.

Last but defently not least: use const wherever you can.

This will make your list less type error prone and will allow it's user to do less stupid stuff with it.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ How do I use const in a class? And for what should I use const here? Also, does the c++ syntax make any performance changes? \$\endgroup\$ – Sahil Arora Apr 8 '16 at 6:26
  • \$\begingroup\$ Const in a functions definition is like a "contract" between the writer of the function to the user. As the writer if you add for instance: f (const int&) it means you guarantee that you won't change the value of the int parameter. There are a lot more ways to use const. I suggest you try to explore yourself. About the syntax, if a class has a private data member it creates an instance of it before you enter the constructor body. Which means that there is a moment where your data members exist but not initialized. It is considered good practice. \$\endgroup\$ – bergerg Apr 8 '16 at 9:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.