HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Optimising LinkedList class - Part 2

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
partclassoptimisinglinkedlist

Problem

This is part 2. My problems are with too any pointers and with long body code in the following functions:

void push_back(T_ data);
void push_front(T_ data);
T_ pop_back();
T_ pop_front();
void insert(iterator_t& i, T_ data);
void erase(iterator_t& i);
T_ front();
T_ back();
void print();
void clear();


```
private:
Node* _head;
Node* _tail;
int _size;

public:
typedef LinkedList list_t;
typedef Node node_t;
typedef list_t* listPtr;
typedef node_t* node_ptr_t;
typedef LinkedListIterator iterator_t;

LinkedList() : _head( nullptr ), _tail( nullptr ), _size( 0 ) {}
LinkedList( LinkedList && list ) { // Rvalue move ctor
_head = std::move(list._head);
_tail = std::move(list._tail);
_size = std::move(list._size);
}
~LinkedList() {
// dtor deletes all of the node ptrs in the list
// very important to avoid memory leaks
while(_head)
{
node_ptr_t tmpNode(_head);
_head = _head->next;
delete tmpNode;
}
}

LinkedList& operator = ( LinkedList && list ) { // Rvalue = operator
_head = std::move(list._head);
_tail = std::move(list._tail);
_size = std::move(list._size);
return *this;
}

bool empty() const { return ( !_head || !_tail ); }
operator bool() const { return !empty(); }

node_ptr_t head() { return _head; }
node_ptr_t tail() { return _tail; }
int size() { return _size; }

iterator_t begin() { return iterator_t(_head); }
iterator_t end() { iterator_t i(_head); while(i != nullptr) ++i; return i; }

void push_back(T_ data){
_tail = new node_t(data, _tail, nullptr);
if( _tail->prev )
_tail->prev->next = _tail;

if( empty() )
_head = _tail;
++_size;
}
void push_front(T_ data)
{
_head = new node_t(data, nullptr, _head);
if( _head->next )

Solution

Size

Normally I'd say right away that size should return an std::size_t instead of an int, but some recent collaborations amongst top C++ experts (including Bjarne Stroustrup himself) have revealed that this can be problematic due to signed/unsigned mismatch. In your implementation at least, you could probably keep it as is, but I at least wanted to bring that up here.

Pointers

In general, keep in mind that you should use smart pointers for C++. They are preferred as they can take ownership of what they point to, which especially ensures that their data will be cleaned up properly.

However, for containers (like you have here), you would use raw pointers. Just make sure to manage them correctly because the list should still be responsible for cleaning up after itself.

clear() and ~LinkedList()

You may not need clear() since you already have to do the same thing in the destructor. If you prefer to be able to clear the nodes at will but keep the same object, then you can keep it. If you choose to keep it, then you can just call clear() in the destructor to avoid duplicate code.

Self-assignment

Normally you (may) do self-assignment with the copy constructor and operator=, but with a move contstructor, you likely will not. A more detailed answer on this can be found here.

Nonetheless, this is what operator= would look like if you were to use it anyway. This would also be similar for the copy constructor.

LinkedList& operator = ( LinkedList && list ) {
    if (this != &list) {
        _head = std::move(list._head);
        _tail = std::move(list._tail);
        _size = std::move(list._size);
    }

    return *this;
}


Misc.

-
Your pop_front() and pop_back() should just be void. This is especially useful as they may potentially fail (especially with templated types), and so you won't be forced to return something. However, popping from an empty container can still cause undefined behavior.

Along with that, you can also have define front() and back() to return references to elements. With a pop_back() that is void, this will allow you to copy to a local variable before the pop.

Note that these are all done in the STL, which you should try to imitate.

-
With begin() and end(), you may also define cbegin() and cend(), which return const iterators. This would be useful for uses such as displaying the list contents.

-
head(), tail(), and size() should be const as they're accessors:

node_ptr_t head() const { return _head; }
node_ptr_t tail() const { return _tail; }
int size() const { return _size; }

Code Snippets

LinkedList& operator = ( LinkedList && list ) {
    if (this != &list) {
        _head = std::move(list._head);
        _tail = std::move(list._tail);
        _size = std::move(list._size);
    }

    return *this;
}
node_ptr_t head() const { return _head; }
node_ptr_t tail() const { return _tail; }
int size() const { return _size; }

Context

StackExchange Code Review Q#41389, answer score: 7

Revisions (0)

No revisions yet.