Initializing values of a singly linked list with a constructor [closed]

Posted on

Problem

I have to create a constructor that allows a new linked list to be populated with ten consecutive values, starting at 0. Then I need to print the list! So i want to check if the functions I wrote for those are ok.

#include <iostream>
#include <string>
#include <stdexcept>

using namespace std;

template <typename E> class SLinkedList;    // forward declaration to be used when declaring SNode

template <typename E>
class SNode {                   
private:
    E elem;                 
    SNode<E> *next;             
    friend class SLinkedList<E>;        
};

template <typename E>
class SLinkedList {             
public:
    SLinkedList();              
    SLinkedList(SNode<E>* v);   //What I need help with
    ~SLinkedList();             
    bool empty() const;         
    E& front();                 
    void printList(SLinkedList<E> &list); //what i need help with
    void addFront(const E& e);      
    void removeFront();         
    int size() const;                   
private:
    SNode<E>* head;             
    int     n;                          // number of items
};

template <typename E>
SLinkedList<E>::SLinkedList()           // constructor
    : head(NULL), n(0) { }

template <typename E>
SLinkedList<E>::SLinkedList(SNode<E>* v){ //WHat I  Need Help With
    SNode<E>* v = new SNode<E>;
    for (int i = 0; i < 10; i++)
        v->elem = i;
}

template <typename E>
bool SLinkedList<E>::empty() const      
{
    return head == NULL; // can also use return (n == 0);
}



template <typename E>
E& SLinkedList<E>::front()      
{
    if (empty()) throw length_error("empty list");
    return head->elem;
}

template <typename E>
SLinkedList<E>::~SLinkedList()          
{
    while (!empty()) removeFront();
}

template<typename E>
void SLinkedList<E>::printList(SLinkedList<E> &list) //What I need help with
{
    for (int i = 0; i < list.size(); i++)
    {
        cout << list << " ";
    }
    cout << endl;
}

template <typename E>
void SLinkedList<E>::addFront(const E& e) { 
    SNode<E>* v = new SNode<E>;     // create new node
    v->elem = e;                // store data
    v->next = head;             // head now follows v
    head = v;               // v is now the head
    n++;
}

template <typename E>
void SLinkedList<E>::removeFront() {        
    if (empty()) throw length_error("empty list");
    SNode<E>* old = head;           
    head = old->next;           
    delete old;             
    n--;
}

template <typename E>
int SLinkedList<E>::size() const {              
    return n;
}

Solution

Don’t do this:

using namespace std;

Bringing everything from std into the global namespace causes more issues than it is worth. I know every book on the subject does it. But they do it because of size limitations in their medium. You should never do this in real code.

See: Why is “using namespace std” considered bad practice?

This should not be a public class:

template <typename E>
class SNode {                   
private:
    E elem;                 
    SNode<E> *next;             
    friend class SLinkedList<E>;        
};

Make it a private member class of SLinkedList. Then it gets smaller. You can make it struct, and you won’t need the friend declaration or the <E> sprinkled everywhere.

That does not look correct:

SLinkedList(SNode<E>* v);   //What I need help with

Why would somebody want to create a linked list with a node? Why are you passing a pointer to the node? What is the ownership semantics of the pointer? So many unanswerable questions. Just get rid of this constructor.

I don’t see any of the normal constructors I would expect for a container that is doing memory management (and you are doing memory management). Please look up the rule of three.

// I would expect to see
SLinkedList(SNode<E> const& copy);
SLinkedList& operator=(SLinkedList const& copy);

For bonus points add move semantics:

SLinkedList(SLinkedList&& move) noexcept;
SLinkedList& operator(SLinkedList&& move) noexcept;
void swap(SLinkedList& other) noexcept;

Yeahh. Stop using NULL; that’s an old hangover for C (we don’t like C here). C++ has a better identifier. Use nullptr; it has better type information associated with it (so it will not be converted to a integer type accidentally).

Sure you can throw.

template <typename E>
E& SLinkedList<E>::front()      
{
    if (empty()) throw length_error("empty list");
    return head->elem;
}

But do you really want to?

I am an expert user and will always check that list is not empty before calling front(). But you are making me pay a penalty (the extra check (I just checked)) because there are idiots out there that don’t check. Why are you penalizing me (the good guy) when the bad guys are the ones you should be penalizing?

Sure have a checking version. But the standard version should not check.

OK. This is obviously not working:

template<typename E>
void SLinkedList<E>::printList(SLinkedList<E> &list) //What I need help with
{
    for (int i = 0; i < list.size(); i++)
    {
        cout << list << " ";
    }
    cout << endl;
}

You want:

template<typename E>
void SLinkedList<E>::printList(SLinkedList<E> &list) //What I need help with
{
    for (auto item = head; item != nullptr; item = item->next)
    {
        cout << item->elem << " ";
    }
    cout << "n";
}

Prefer 'n' to std::endl.
The only difference between the two is that std::endl forces a stream to flush. This is never the correct thing to do (unless you are an expert and understand stream flushing to byte level (or you are debugging – in which case you should be using an unbuffered stream like std::cerr)).

Incorrectly flushing the buffer is the MAIN cause of inefficiency in C++ I/O application. The standard streams are quite good at correctly flushing for efficiency; use the code somebody else has spent hours perfecting for your system.

This all looks fine:

SNode<E>* v = new SNode<E>;     // create new node
v->elem = e;                // store data
v->next = head;             // head now follows v
head = v;               // v is now the head
n++;

But would it not look a lot nicer if it was:

head = new SNode(e, head);
++n;

Also prefer to use prefix increment over postfix. The majority of the time it will make no difference. But with iterators (it can have a tiny impact). Since iterators are prevalent all over C++ code this could make a difference. There is no cost to prefix operations.

See: How to overload the operator++ in two different ways for postfix a++ and prefix ++a?

Additions

I have to create a constructor that allows a new linked list to be populated with ten consecutive values, starting at 0.

A special constructor for adding ten items is a bit strange. But we can do it. Let’s create a marker enum:

enum Marker { MakeTenItems };
SLinkedList(Marker)
    : head(nullptr)
{
    for(int loop = 0; loop < 10; ++loop)
    {
        addFront(10 - 1 - loop);  // This will construct an E with an int
 // So E must have an appropriate constructor.
    }
}

Then to use it:

SLinkedList<int>  list(MakeTenItems);

Leave a Reply

Your email address will not be published. Required fields are marked *