current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

I created a simple linked list to test what I've learned so far and if I manage everything correctly. If you see syntax errors is't because of copy-paste. I just want to know if i'm making everything in the right way without having leak and memory problems.

Node class

#include "Node.h"


//Constructor.
Node::Node(int v)
:next(nullptr), value(v)
{
}


//Deconstrucor (not in use).
Node::~Node()
{
}


//Set next.
void Node::set_next(Node *new_node)
{
    this -> next = new_node;
}


//Set previous.
void Node::set_prev(Node *new_node)
{
    this -> prev = new_node;
}


//Set value.
void Node::set_value(int v)
{
    this -> value = v;
}


//Get next.
Node *Node::get_next()
{
    return this -> next;
}


//Get previous.
Node *Node::get_prev()
{
   return this -> prev;
}


//Get value
int Node::get_value()
{
    return this -> value;
}

LinkedList class

#include "LinkedList.h"
#include "Node.h"
#include <new>
#include <iostream>


//Constructor.
LinkedList::LinkedList()
:head(nullptr), tail(nullptr), moving_ptr(nullptr)
{
}


//Deconstrucor (not in use).
LinkedList::~LinkedList()
{
}


//Append a new item on the list.
void LinkedList::append(int v)
{

    //Allocate a new node.
    Node *new_node = new (std::nothrow)Node(v);


    //If memory is full, exit the program.
    if (new_node == nullptr){
        std::cout << "Memory is full, program will exit." << std::endl;
        exit(0);
    }


    //If this is the first node.
    if (head == nullptr || tail == nullptr)
        {
            head       = new_node;
            tail       = new_node;
            moving_ptr = new_node;
        }


    //Append other node.
    else
        {
            tail -> set_next(new_node); //Link it after the tail of the list.
            new_node -> set_prev(tail); //Set the previous node.
            tail = new_node;            //Update the tail pointer.
        }
}



//Remove a node from the list.
bool LinkedList::remove_item(int v)
{

    //Starting node.
    Node *curr = head;


    //----Find the node that contains v----//
    while (curr != nullptr)
    {

        //Found.
        if (curr -> get_value() == v)
            break;


        //Keep going.
        else
            curr = curr -> get_next();
    }
    //----Find the node that contains v----//



//The item was not found into the list.
if (curr == nullptr)
    return false;



//Make necessary links!!!
curr -> get_prev() -> set_next( curr -> get_next() );


//Delete current node.
delete curr;

return true;
}



//Get the next item.
int LinkedList::nextItem()
{

    //Temp that hold current value.
    int temp = moving_ptr -> get_value();


    //Move to the next node.
    moving_ptr = moving_ptr -> get_next();

    //Return the value.
    return temp;
}


//Return true if moving_ptr is not null.
bool LinkedList::hasNext()
{
    return moving_ptr != nullptr;
}


//Reset moving_ptr.
void LinkedList::reset()
{
    moving_ptr = head;
}

Am I handling everything correctly? I also tried to fill all my memory with an infinite loop to see what happens and everything run's smoothly. The program terminates when memory is full.

share|improve this question

closed as off-topic by Loki Astari, syb0rg, Dannnno, Quill, Mast Jul 16 at 9:49

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.
    
As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. – syb0rg Jul 15 at 14:49
2  
Are you aware that C++ has a linked list container type? cplusplus.com/reference/list/list – pacmaninbw Jul 15 at 14:51
1  
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. – forsvarir Jul 15 at 15:15
3  
Without the types of the members this is impossible to review. In C++ the most important part is the type information. So without the class declarations this is impossible to review. Voting to close. – Loki Astari Jul 15 at 15:42
1  
Agree with @LokiAstari, we need definition of the classes. Namely .h files. With it we can give you much more concrete review. – Olzhas Zhumabek Jul 15 at 20:56
up vote 0 down vote

Memory Leaks

You aren't cleaning up after your LinkedList. When the class is destroyed you want to make sure that all of the memory it has allocated is cleaned up. At a minimum, you're going to want to:

LinkedList::~LinkedList()
{
    delete head;
}

However you also need to consider each node in the list. You can either have ~Node delete it's next, or you can put a loop in to delete all the items in the list from the tail backwards. Usually you would also implement this in a public function like clear or flush so that the whole list can be easily emptied. For example, something like this would clear up the list:

void clearList() {
    Node *tmp = head;
    while(tmp) {
        head = tmp->get_next();
        delete tmp;
        tmp = head;
    }
}

Exit

You exit if there is insufficient memory to allocate a node. The exitcode 0 is usually reserved for a successful program exit. You should be passing a different value to exit (usually a negative number) to indicate failure.

Heads or Tails

In your remove_item method, you're not updating your tail pointer, what is going to happen if the tail happens to be pointing at the node that contains the value you remove? Similarly, what happens if moving_ptr is pointing at the node that's removed?

reset

I'd honestly expect this to clear/flush/empty the list, not just reset the pointer that's used for iterating over items in the list. This is likely to be confusing.

templates

Depending on what it is you're practising for, a good next step would be to adapt your Node so that it can support a templated value type. You could then extend the list to store and retrieve values of any type.

share|improve this answer
    
fare enough :p Ok i know what do u mean by cleaning up all the nodes. Also i will correct the exit state. Thanks :) – babaliaris Jul 15 at 15:04
    
OMG you are right about the remove method, didn't notice that :p Thank's :p – babaliaris Jul 15 at 15:07
    
@babaliaris I've added an example of clearList, which could be called in the destructor for the list. I haven't tested it, but it should point you in the right direction. – forsvarir Jul 15 at 15:11
1  
@forsvarir: Not if it was std::auto_ptr. The point is there are a million different things that could happen. You are just guessing. Without type information (the most important thing in C++) your ability to review the code is inhibited completely and your advice could be correct or completely wrong. – Loki Astari Jul 15 at 15:58
1  
@LokiAstari I believe auto_ptr would have caused the compiler to barf on Node *curr = head, so I'm still comfortable believing that based on the supplied code it is using a raw pointer and that my advice is valid. However I take your point. – forsvarir Jul 15 at 16:05

Not the answer you're looking for? Browse other questions tagged or ask your own question.