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

Singly linked-list with smart pointers

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

Problem

A few things:

  • I tried to do it with smart pointers because I wanted to learn about them. I'm not sure I made the right choice of type, however (and started to regret it half-way through).



  • This is for school, so some operations were required. I can't change main.cpp or the general layout.



  • insert() works. However, I'm suspicious it's not working properly. I haven't implemented delete functionality yet.



Any comments / suggestions appreciated.

Here's my singly linked list.

I've also posted this on Reddit.

List.cpp

#include "List.h"

void List::reverse_print(std::shared_ptr curr) const {
    if (curr) {
        reverse_print(curr->next);
        std::cout datum  new_link(new Link(val));
    new_link->next = root;
    root = std::move(new_link);
}

void List::push_back(int val) {
    std::unique_ptr new_link(new Link(val));

    if (!root)
        root = std::move(new_link);

    else {
        std::shared_ptr curr(root);

        while (curr->next) {
            curr = curr->next;
        }

        curr->next = std::move(new_link);
    }
}

void List::insert(int val, int loc){

    if (loc > length( ) + 1 || loc  ptr(root);

            while (node_count next;
                ++node_count;
            }

            std::shared_ptr new_link(new Link(val));
            ptr->next = new_link;
        }
    }
}

int List::length() const { 

    int node_count = 0;
    std::shared_ptr ptr(root);

    while (ptr) {
        ptr = ptr->next;
        ++node_count;
    }

    return node_count;
}

void List::print() const {
    std::shared_ptr temp(root);

    if (!temp) {
        std::cout datum next;
    }

    std::cout  List::get_first() const {
    return root;
}

void List::destroy_list() {
    root.reset();
};

bool List::empty( ) const {
    return root == nullptr;
}

/*
void delete_link( ){ }
void delete_first( ) { }
void delete_last( ) { }
*/


List.h

```
#ifndef LIST_H
#define LIST_H

#include
#include

class List {
struct Link {

Solution

Some comments to the things you have implemented so far:

  • reverse_print: Printing the list in reverse order recursively is an academically interesting solution but if your list is sufficiently long then your stack will explode. Use a std::stack to temporarily store the elements and then print that.



  • push_back: Currently this is O(n) because you iterate the entire list to find the last element. If you store a tail pointer which points to the last element in the list in addition to root then you can avoid that and this operation becomes O(1).



  • length(): Keep a length property in the class which you increment/decrement when a node got added/removed. Again reduces an O(n) operation to O(1).



  • insert():



  • While it's commendable for the (application) user experience to have 1 based indices, as a programer who might use this it will be confusing. Indices into containers like arrays and vectors are 0 based in C++ land. Deviating from the standard is bad because it is unexpected.



  • Insert is broken. You will lose all items after the insert position (new_link->next points to nothing and ptr->next now points to new_link -> all elements starting from the old ptr->next are now lost)

Context

StackExchange Code Review Q#33136, answer score: 5

Revisions (0)

No revisions yet.