Thread: Linked List question from a newb(Need code Critique)

Hi!

If someone has a few minutes, I could really use critique on the following code. I understand Linked Lists, and I believe I understand how to implement it, but my attempts in school tended to generate fatal errors, and my text is somewhat...poor at giving code examples.

The goal of the code is to create a linked list of items with a string for a name, an ID number(techID), and 3 int parameters. I'll also need to use several methods on the data, some of which are too long for in-line definition.

The code compiles correctly in VC++.net 2003, and seems to work properly(Read it didn't crash), but I still feel very unsure about it.

The header file

Code:

#include <iostream>
#include <vector>
#include <string>

using namespace std;

class techNode
{
public:
	techNode();
	techNode(string tName, int tID, int miDmg, int maDmg, int spec,
		techNode* theLink);
	techNode* getLink() const ;
	string getName() const ;
	int getID() const;
	vector<int> getStats() const;
	void setData(string tName, int tID, int miDmg, int maDmg, int spec,
				techNode* theLink);
	void setLink(techNode* pointer);

private:
	string name;
	int techID;
	int minDmg;
	int maxDmg;
	int specials;
	techNode* link;
};
typedef techNode* techNodePtr;

void headInstert(techNodePtr& head, string tName, int tID, int miDmg, int maDmg,
				 int spec);

void insert(techNodePtr afterMe, string tName, int tID, int miDmg, int maDmg,
			int spec);

The Implementation file

Code:

#include <iostream>
#include <vector>
#include <string>
#include "techNode.h"

using namespace std;

techNode::techNode()
{
}

techNode::techNode(string tName, int tID, int miDmg, int maDmg, int spec, 
				   techNode *theLink)
{
	name = tName;
	techID = tID;
	minDmg = miDmg;
	maxDmg = maDmg;
	specials = spec;
	link = theLink;
}

techNode* techNode::getLink() const
{
	return link;
}

string techNode::getName() const
{
	return name;
}

int techNode::getID() const
{
	return techID;
}

vector<int> techNode::getStats() const
{
	vector<int> tempStats;
	tempStats.push_back(minDmg);
	tempStats.push_back(maxDmg);
	tempStats.push_back(specials);

	return tempStats;
}

void techNode::setData(string tName, int tID, int miDmg, int maDmg, int spec,
					   techNode* theLink)
{
	name = tName;
	techID = tID;
	minDmg = miDmg;
	maxDmg = maDmg;
	specials = spec;
	link = theLink;
}

void techNode::setLink(techNode* pointer)
{
	link = pointer;
}

void headInstert(techNodePtr& head, string tName, int tID, int miDmg, int maDmg,
				 int spec)
{
	head = new techNode(tName, tID, miDmg, maDmg, spec,	head);
}

void insert(techNodePtr afterMe, string tName, int tID, int miDmg, int maDmg,
			int spec)
{
	afterMe->setLink(new techNode(tName, tID, miDmg, maDmg, spec, afterMe->getLink()));
}

Thanks in advance if anyone could take a look at it!

You should use initialiser lists here:

Code:

techNode::techNode(string tName, int tID, int miDmg, int maDmg, int spec, 
				   techNode *theLink) :
	name(tName), techID(tID), minDmg(miDmg),
	maxDmg(maDmg), specials(spec), link(theLink)
{}

You could perhaps pass string's by const reference?
Oh and you have a spelling mistake on one of your function names: "headInstert".

Other than that, it looks fine and should work okay too.

> using namespace std;
Never put this inside header files.
Anyone who includes your header automatically gets the std namespace along with it otherwise, and that's simply bad news if you didn't want that.

I would put
string name;
int techID;
int minDmg;
int maxDmg;
int specials;
into a separate structure/class.
This would be the start of separating the idea of 'list' from 'data'. That is, the list shouldn't care what it is a list of. It would also shorten the parameter lists of your list data manipulation functions.

Several places use pointers, where references would have been cleaner.