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

General advice on a practice linked_list for C++ classes/templates

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

Problem

Introduction

I'm learning C++ (Coming from Haskell, C, and Assembly - and other languages sparsely) and this was my practice with classes and templates. It's a linked list that you can call in this fashion:

#include "list.hpp"

int main(void){
   linked_list test;

   char c;

   while(c != '\n') //
      test.prepend(c = fgetc(stdin));

   std::cout << test;

   std::cout << std::endl;

   return 0;
}


Topic

I am here to ask if some more experienced coders would point out my mistakes (mainly about classes) so I can break the habits. Thanks for your help!

Here is the source code: (Btw, it isn't optimized on purpose, I was only playing with classes/templates)

(I do notice the inefficient and potentially dangerous tail() call, but considering the trivial nature of the project - I decided to leave it for later, simply make it a deque. I also naively checked it with memcheck. No leaks and no errors in my library, seemingly.)
Ideone Link

```
//Compile with "g++ -std=C++11 -O2 -o list main.cpp list.hpp"

#ifndef LIST_H
#define LIST_H
#include

template
class linked_list {
private:
int empty = 1;

struct node {
T data;
struct node * next;
} * head;

struct node * get_add(int index);
public:
linked_list(){head = new node;};
linked_list(T data){head = new node; head = data; empty = 0;}
~linked_list();

int size();
inline int nsize(){return sizeof(struct node);};
inline struct node * lhead() {return head;};
inline struct node * tail() {return get_add(size()-1);};

int search(T key);

void prepend(T data);
void append(T data);
int insert(T data, int index);
int del(int index);

int set(T data, int index);
T get(int index);

T& operator[](int i);
};

template
std::ostream& operator &list);

//////////////IMPLEMENTATION

Solution

This won't be as complete an answer as you often see on Code Review, but here are a few pointers to improve your code:

template 
T& linked_list::operator[](int i){
    return (get_add(i)->data);
}


First of all, you should tell the programmer that your index will never be a negative one. In order to do so, you could change your int parameter to be an unsigned int one. Here, since we are talking about sizes, a size_t or std::size_t parameter would be even better:

template 
T& linked_list::operator[](std::size_t i) {
    return get_add(i)->data;
}


C++ differentiates between functions that change the class members and functions that do not. If your function does not change the member variables, it should be const-qualified. Moreover, this qualification is a part of the function's signature. That means that you can have both the const-qualified version of the function in your class and the normal one:

template 
T& linked_list::operator[](std::size_t i) {
    return get_add(i)->data;
}

template 
const T& linked_list::operator[](std::size_t i) const {
    return get_add(i)->data;
}


You can apply my remarks about const-qualified functions to every function that should not change the class (e.g. size and search).

Now, let's talk about this piece of code:

template 
std::ostream& operator &list) { ... }


Let's have a look a the function's signature: your parameter is a linked_list&. However, you should specify that this output function will not modify your list by adding a const before the type:

template 
std::ostream& operator &list) { ... }


About for loops in C++:

int i;
for(i = 0; i < list.size(); i++)
    out << list.get(i);


In C++, you can use declaration statements in the first part of the for. That means that you don't have to declare your int i before the loop. You can declare it inside the for. And that is what should be done, unless you still need the value of i after you leave the loop. You can write it like this:

for(int i = 0; i < list.size(); i++)
    out << list.get(i);


And since your list can not have negative indices, the std::size_t is still welcome:

for(std::size_t i = 0; i < list.size(); i++)
    out << list.get(i);


We will now see this piece of code:

int search(T key);


While it is valid, you are passing key by value, which means that if you pass an instance of a big class (if T is a big class), it will be entirely copied before being used. In C++, if we don't need to modify the object passed to a function, it is better to pass it by const reference: it will not change the way you use it, but the compiler will use the address of the instance instead of doing a whole copy. To search, you don't need to modify your key, so it is better to pass by const reference:

int search(const T& key);

Code Snippets

template <typename T>
T& linked_list<T>::operator[](int i){
    return (get_add(i)->data);
}
template <typename T>
T& linked_list<T>::operator[](std::size_t i) {
    return get_add(i)->data;
}
template <typename T>
T& linked_list<T>::operator[](std::size_t i) {
    return get_add(i)->data;
}

template <typename T>
const T& linked_list<T>::operator[](std::size_t i) const {
    return get_add(i)->data;
}
template <typename T>
std::ostream& operator<< (std::ostream &out, linked_list<T> &list) { ... }
template <typename T>
std::ostream& operator<< (std::ostream &out, const linked_list<T> &list) { ... }

Context

StackExchange Code Review Q#25283, answer score: 8

Revisions (0)

No revisions yet.