patterncppMinor

Templated Singly-Linked List

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

singlytemplatedlinkedlist

Problem

I'm really new to C++ (very surprised at the leap from Python and others to C++!). The code compiles and works well. This took all day to get to work, and I've the tendency to move on after I get [what I see as] basic functionality. I'd really like to become a good programmer, though.

I assume there are a ton of things I'm doing wrong, but I don't know what they are.

Although I've read a lot about them, I still don't fully understand when I should use a pointer. I know what they are, and what its difference is with a reference—but I haven't grokked why I should use one over the other.

Also, everything I've read about implementing the STL iterator (and I've read quite a bit on that) confuses the hell out of me, so I wrote a pretty basic one instead.

```
//====================
// Include Guard
#ifndef __LINKEDLIST_HPP_INCLUDED__
#define __LINKEDLIST_HPP_INCLUDED__

//===================
// Included Dependencies
#include
#include

//====================
// Forwarded Dependencies

//====================
// Classes (Header)

template class Node {
public:
Node();
Node(T object);
T getObject();
Node* next_;

private:
T object_;
};

template class ListIterator {
public:
ListIterator(Node* head_node);
T next();

private:
Node* current_node;
};

template struct LinkedList {
public:
LinkedList (T initialObject);
~LinkedList();
void addObject (T addedObject);
void removeObject (int position);
int getSize();
ListIterator getIterator();

private:
Node* head_;
Node* current_;
int size_;
};

//====================
// Class (Instantiation)

template Node::Node() {};
template Node::Node(T object) { object_ = object; };
template T Node::getObject() { return object_; };

template ListIterator::ListIterator(Node* head_node) {
current_node = head_node;
};
template T ListIterator::next() {
Node* tmp_node = current_no

Solution

Node

There is no need to make Node public. LinkedList and ListIterator need it but client of your library doesn't. So consider hiding it from him. For example by defining Node within the LinkedList class.

Node()

It would be cleaner if next_ would be set to nullptr (if in C++11 or above) or NULL/0 (otherwise).

There should be no use for default constructor anyway. (See below notes on LinkedList::head_.) In fact use of this constructor requires type T to be DefaultConstructible while there is no need to require that.

Node(T object)

Use member initializer list instead of assigning to members in constructor's body.

It would be cleaner if next_ would be set to nullptr (if in C++11 or above) or NULL/0 (otherwise).

Change argument to const T& if you are pre-C++11.

Otherwise use move semantics when constructing object_ from object. And also possibly add const T& overload anyway.

getObject()

Return T& instead of T as it would not require copying of the object.

And add const overload which returns const T&.

next_

Should not be a public member! Make it private and grant to LinkedList and ListIterator access to for example using friend.

It could be useful to forbid copying of Node objects as it seems there is no use in that. To do that use boost::noncopyable if you have Boost. If you don't then either explicitly "delete" copy constructor and assignment operator (if in C++11 or above) or make them private without defining them.

ListIterator

As you noted yourself your iterator differs significantly from what C++ considers and iterator. This means in particular that it will not be usable with any STL function. Or other libraries that use "normal iterators".

As a side note I will mention that boost::iterator_facade makes it much easier to write proper iterator. But it requires use of Boost.

With your current design as it is how will you know that the iteration ended? There is no method in ListIterator that says that. With your current implementation (of cyclic list - see comments in LinkedList) any iteration would be infinite unless you would count elements yourself during iteration and stop at getSize.

ListIterator(Node* head_node)

Use member initializer list instead of assigning to members in constructor's body.

next()

Implementation incorrectly returns value from current_node. It seems that instead it should return value from tmp_node.

Return T& instead of T as it would not require copying of the object. (But to do that you have to do the same for Node::getObject() as commented above.)

Consider having also a ConstListIterator which uses const Node* and returns const T& from next(). This would allow to iterate over const LinkedList object. With some template magic this could be done with single implementation.

LinkedList

From implementation it seems to be a cyclic list. Was this intended?

I don't follow the idea behind head_ node. I think that it is not needed. Not to mention that it's value could be undefined while it will show up during iteration.

The class would be a bit more usable if you could add element at arbitrary position. Given by integer (to make your design consistent) or iterator (to be consistent with previous comment).

The class would be a bit more usable if you could remove element based on iterator. Or else allow the iterator to return it's position as integer. But remove by iterator is more in line with C++ (STL) style.

The class would be a bit more usable if it allowed also const iteration. So getIterator() should have a const overload returning const iterator (as commneted above).

Size in C++ is usually expressed using std::size_t type. So should be also positions in your case.

There should be a default constructor making an empty list.

LinkedList(T initialObject)

Consider making it explicit so that it cannot be used for conversion.

Add also second argument being count of initial elements. And default it to 1. It would make the constructor a bit more flexible and little cost. (And also it will match typical STL container constructors.)

Change argument to const T& if you are pre-C++11.

Otherwise use move semantics when constructing object_ from object. And also possibly add const T& overload anyway.

Use member initializer list instead of assigning to members in constructor's body.

You should properly define copy constructor and assignment operator. Or at least forbid them (as already mentioned for Node).

Consider adding other constructors. For example from a range of elements of other list (by positions or by iterators).

~LinkedList()

In loop use ++ii rather than ii++. On modern compilers with integers it doesn't matter really. But it is cleaner to write so. And could make a difference if ii` would be an iterator rather than inte

Context

StackExchange Code Review Q#100782, answer score: 5

Revisions (0)

No revisions yet.