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

3-class linked list

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

Problem

I am trying to make a linked list using 3 classes, however I cannot find ANY information online about it, and I can't seem to figure it out myself.

If you could please correct and explain what I am doing wrong, I would greatly appreciate it.

LinkTest.h:

#include 

class Test
{
public:
    void get(int &, std::string &);
private:
    int int1;
    std::string string1;
};

typedef class TestNode
{
    private:
        Test test;
        TestNode* next;
} *TestPtr;

class TestList
{
    public:
        TestList();
        void add(int &, std::string &);
        void print();
    private:
        TestPtr head;
        TestPtr tail;
        TestPtr temp;
};


LinkTest.cpp:

#include 
#include "LinkTest.h"
using namespace std;

void Test::get(int &otherint1, string &otherstring1)
{
    int1 = otherint1;
    string1 = otherstring1;
}

TestList::TestList()
{
    head = NULL;
    tail = NULL;
    temp = NULL;
}

void TestList::add(int &otherint1, string &otherstring1)
{
    TestPtr* n = new TestPtr;
    n->next = NULL;
    n->get(otherint1, otherstring1);

    if(head != NULL)
    {
        tail = head;
        while(tail->next != NULL)
        {
            tail = tail->next;
        }
        tail->next = n;
    }
    else
    {
        head = n;
    }
}

void TestList::print()
{
    tail = head;
    while(tail != NULL)
    {
        cout test.int1 test.string1 next;
    }
}


The main function just does:

int otherint1 = 1;
string otherstring1 = "hi";
TestList l;
l.add(otherint1, otherstring1);
otherint1 = 2;
otherstring1 = "there";
l.add(otherint1, otherstring1);
l.print();

Solution

At first glance, your code appears to work, but there is some confusion going on. I'll highlight what I consider to be the four top issues instead of making an exhaustive list.

  • The Test::get() method is actually a setter, not a getter, and should be named Test::set().



-
The Test class seems to represent the kind of data you want to store in the list. You should just eliminate Test in favour of std::pair.

The linked list code doesn't really have any business providing an add() method that takes two kinds of data to be stuffed into the new node. Instead of calling TestList::add(int, std::string), just make the construction explicit to the caller, as TestList::add(std::pair(someInt, someString)).

  • TestList has three member variables: head, tail, and temp. Of those three, only head stores object state and deserves to be a member variable. tail and temp should just be local variables within functions that need them.



  • You call new, but it's not paired with a delete anywhere. That's a memory leak. The memory that you allocate should be cleaned up in a destructor.

Context

StackExchange Code Review Q#46362, answer score: 6

Revisions (0)

No revisions yet.