Implementation of a singly linked list in C++

This is a follow-up to the previous question: Implementation of the linked list data structure in C++

I've tried to improve the design of the previous code by fixing some indexes that could go out of range and cause the dereferencing of a nullptr, and minor changes for compactness and elegance. However, I'm not sure about the final result (in all honesty I'm satisfied because this is my second week working with C++), so that's why I'm asking for feedback.

```
#ifndef NYO_UTIL_LINKEDLIST_H
#define NYO_UTIL_LINKEDLIST_H

#include
#include

template using ptr = std::shared_ptr;

namespace Nyo{
namespace Util{

template
class LinkedList
{
private:

class Node
{
private:
V data;
ptr next;
public:
Node(const V& _data) : next{}, data{_data} {}
ptr& getNext(){
return this->next;
}
V getData(){
return this->data;
}
};

ptr head;
ptr tail;
size_t _size;

public:

LinkedList() : head{}, tail {}, _size{} {}

void add(const V& _data){

auto _tmp = std::make_shared(_data);

if(isEmpty()){

head = (_tmp);
tail = (_tmp);

} else {
tail->getNext() = (_tmp);
tail = (_tmp);
}
_size++;
}

bool isEmpty(){
return (head==nullptr);
}

V operator[](int index){

if(index size()-1){ return {};}

else {

int _c {};
ptr tmp = (head);

while(tmp!=nullptr){

if(_c == index){
break;
}
tmp = (tmp->getNext());
_c++;
}

return tmp->getData();
}
}

void pushFront(const V& _data){

auto _tmp = std::make_shared(_data);

if(isEmpty()){
_tmp->getNext() = (head);
head = (_tmp);
tail = (_tmp);

} else {
_tmp->getNext() = (head);
head = (_tmp);
}

_size++;

}

template
void insertAt(int index, const V& _data){

auto _tmp = std::make_shared(_data);
std::shared_ptr _curr;
std::shared_ptr _afterIndex;

if(index siz

namespace Usage

This type ptr is in the global namespace. Why not make it part of your namespace? All your definitions should be in your namespace.

template  using ptr = std::shared_ptr;

I note you are still using shared pointers for memory management.

Identifier Nameing

I hate the use of _ as a prefix to an identifier. Though you are not actually breaking any rules. Do you actually know the rules about using a prefix _ on an identifier.

Personally I find that people that use _ are using it to help them identify certain names for certain purposes. Personally I see this as a crutch to actual using good meaningful names for these identifiers.

Style

This looks funny.

tail = (_tmp); // why the braces?

Prefer prefix increment

_size++;

When using integer it makes absolutely no difference. But when you are using user defined types (like iterators) it can. The default implementation of an iterator the prefix increment is more efficient than the postfix version.

Also you want to write your code so that it is type agnostic. If during maintenance you decide to change a type you should not also have to run through the code making sure that you are doing things the most efficient way (that should be automatic; just change the type).

As a result it is preferable to train yourself to always use the prefix increment (make it a habit). That way you will always be using the most efficient version of increment no matter what the type and situation.

Default Constructing V

You fixed the bug in you access operator operator[]. But you have done so in a way that means your type V must be default constructable.

if(index  size()-1){ return {};}

You are constructing a temporary V object that is returned.

Accessing beyond the end is an error you should treat it as such. Now you can treat this in several ways. The easiest way is to to throw an exception. This is the way I would recomend for you at the moment:

if(index  size()-1)
 {
     throw std::out_of_range("Error Message");
 }

The standard library for vector does it slightly differently. In the vector it is just undefined behavior to use an out of range index. But that fact is well documented (unlike your original version). But they also provide a second method at() that does do range checking. This version will throw an exception when the range is exceeded.

V& operator[](std::size_t index) {return data[index];}
  V& at(std::size_t index)         {checkRange(index);return data[index];}

Returning by reference.

The standard containers return reference to their members. This allows you to modify the members in place in the container.

std::vector   myData(/* Fill the container*/);

  myData[5] = 56; // modifies the value in the container.
                  // this only works if you return a reference.

Access operator const version

Now you have access to members via operator[]. But the contents become inaccessible if you have a const reference to your container. But as long as you are not modifying the container you should still be able to read them.

So you should also add const version of the access operator.

V&       operator[](std::size_t index)       {return data[index];}
  V const& operator[](std::size_t index) const {return data[index];}

Prefer local objects to dynamic objects

auto list = std::make_unique>();

You should only be making objects dynamically like this if you can not tell the lifespan of the object until runtime.

Most of the time you should be using automatic objects.

Nyo::Util::LinkedLis  list;  // This is how you normally declare variables.

StackExchange Code Review Q#140324, answer score: 3