correct behavour of at()

[sh3rpa]

I wanted to build this list ground up again. I'd like to know if I'm on a better track this time?

So far, my example really only tests Iter creation/copying/assigning.


1) Is my Iter assignment operator correct?

2) Is this how the funtion at() is normally implemented?

AList.h:

#ifndef ALIST_H
#define ALIST_H

template <typename T>
class List{
    
    private:
      
        struct Node{
            
            T data;
            Node *next, *prev;
            Node(const T& d, Node* n = 0, Node* p = 0) : data(d), next(n), prev(p) {};
            Node(const Node& src) : data(src.data), next(src.next), prev(src.prev) {};
        };
        
    Node *head, *tail;    
        
    unsigned int sizeOfList;
        
    public:
        
        class Iter{
            
            private:
                
                Node* curr;
                
            public:
                
                Iter(Node* c = 0) : curr(c) {};
                Iter(const Iter& rhs_i) : curr(rhs_i.curr) {};
                
                Iter& operator=(const Iter& rhs_i){ Node* c = rhs_i.curr; return *this; };
                
                const T& operator*() const { return curr->data; };
                T& operator*() { return curr->data; };
                
                bool operator!=(const Iter& i) { return i.curr != curr; };
                bool operator==(const Iter& i) { return i.curr == curr; };
                
                Iter& operator++(int) { curr = curr->next; return *this; };
                Iter& operator--(int) { curr = curr->prev; return *this; };
                
        };
        
    List<T>();
    List<T>(const List& src);
    unsigned int size(){ return sizeOfList; };
    
    const Iter begin() const { return Iter(head); };
    const Iter end() const { return Iter(0); };
    const Iter rbegin() const { return Iter(tail); };
    const Iter rend() const { return Iter(0); };
    
    
    /*  Here's at()
    */
    const Iter at(int n) const {
    
    	Iter temp = begin();
			for(int i = 0;  i < n; i++){
				
				temp++;
			}
			
			return temp;
        }
        
};

template <typename T>
List<T>::List() : head(0), tail(0), sizeOfList(0) {}

#endif

AMain.cpp:

#include <iostream>
#include "AList.h"

int main(){
    
    List<int> mlist;
    
    List<int>::Iter i;
    List<int>::Iter j(i);
    List<int>::Iter k;
    List<int>::Iter l = k = j;
    
    return 0;
}

[anon]

 Iter& operator=(const Iter& rhs_i){ Node* c = rhs_i.curr; return *this; };

You are assigning to a temporary.

I don't think either Node nor Iter needs a special copy contructor or assignment operator. They don't require anything else but the shallow copy that the compiler would produce automatically.

[iMalc]

You have incorrectly implemented postincrement as if it were preincrement, so the semantics of it is wrong.

There should be no "at" function for a list. Any O(n) running time list accessor is fundamentally wrong, as it encourages writing horribly inefficient code. This is why the SC++L does not have it for a list.

[CornedBee]

I think Node should be noncopyable. It doesn't make sense to copy nodes.

[sh3rpa]

I don't think either Node nor Iter needs a special copy contructor or assignment operator. They don't require anything else but the shallow copy that the compiler would produce automatically.

There should be no "at" function for a list. Any O(n) running time list accessor is fundamentally wrong, as it encourages writing horribly inefficient code. This is why the SC++L does not have it for a list.

So on both accounts just remove and implement w/out?

[sh3rpa]

I think Node should be noncopyable. It doesn't make sense to copy nodes.

Is there a special way of preventing the copying of Nodes?

[laserlight]

Make the copy constructor and copy assignment operator private.

[iMalc]

So on both accounts just remove and implement w/out?

yep, yep.

[brewbuck]

AAAck.. I would not provide an at() function for a list. You run the risk of somebody attempting to use it!

EDIT: Seems I was beaten to the punch.

[CornedBee]

Make the copy constructor and copy assignment operator private.

And don't implement them.


Alternatively, derive from boost::noncopyable.