Test is not really a test class.
void get(int &, std::string &);
That's confusing. This is the type of data that you store in the list. Please rename it more appropriately. Common omong containers is
TestNode the naming thing.
typedef class TestNode
This is not your grandpa's C; this is C++. struct live in the same namespace as other types and objects. Also multiple declarations on the same line is frowned upon so split the above into multiple declarations to make it clear wheat is happening.
typedef TestNode* TestPtr;
// ^^^^^^^^^ Note * on left side.
// The type you are defing is a TestNode pointer => New Name TestPre
Personally I don't like hiding pointers behind typedefs. It hides pointers that I want to see because memory management needs to be done with them. I would personally drop the typedef.
Test in the name.
Rather than adding two different types that have nothing to do with your list.
void add(int &, std::string &);
You should add (or push) the data that is held by your list. In this case
// pass a test object that is copied into the list.
void push_back(Test const& );
In modern versions of C++ we now use the term
emplace for this you pass the parameters that can be used in the constructor of your data class. Unfortunately your data class has no constructor (apart from the default).
public: Test(int v, std::string const& x)
// Pass the values for the constructor of a Test object.
// The object will be created emplace using the constructor.
// rather than the copy constructor used by push_front()
void emplace_back(int const&, std::string const&)
Having a print function is fine (but you should pass a stream to print it on).
void print(std::ostream& s = std::cout); // default to std::cout
// if not explicitly specified.
But you should probably also define an
operator<< for your class that calls it. Most people basically do away with the print and just put the logic of the print in
operator>> and make it a friend of the class.
std::ostream& operator<<(std::ostream& s, TestList& data)
Include from the most specific to the most general.
The most specific is the header file for this class. Followed by any header files for classes that you specifically use followed by C++ libraries )as these are general files but are built on-top of C files) followed by C libraries (as these are the most general system files).
Don't do this:
using namespace std;
see Why is “using namespace std;” considered bad practice?
Obviously not a get()
void Test::get(int &otherint1, string &otherstring1)
int1 = otherint1;
string1 = otherstring1;
This is setting the values. It looks more like a constructor than a get. Pass by
const& this is a more healthy contract the promises you will not modify the passed parameters (it also allows you to use temporaries).
void TestList::add(int &otherint1, string &otherstring1)
Yep you should have a constructor that does all this work.
TestPtr* n = new TestPtr;
n->next = NULL;
Would have looked much nicer as:
TestPtr* n = new TestPtr(otherint1, otherstring1);
PS. These are horrible parameter names and do not help in self documenting code.
tail value is a member of the class. Why it it not maintained as always pointing at the last element? Then you would not need to search for the last element every time you add something new to the class.
tail = head;
while(tail->next != NULL)
tail = tail->next;
tail->next = n;
The complexities of maintaining a list are reduced considerably if you use a
sentinel value. See: Linked List reversal
Then you don't need to worry about when the head is NULL (because it never is).
head = n;
Also you don't handle memory management.
Technically for every
new there should be a call to
delete. But that's old school thinking. There should never be any
delete specifically in your code. You should be using smart pointers to manage your pointers so that they are exception safe.
// Don't use tail here.
// tail should always point at the last element in the list.
// Only moe it when there is a new last element.
tail = head;
// here use a local function scope temporary (call it printLoop or something).
while(tail != NULL)
std::cout << std::endl;
// is equivalent to:
std::cout << '\n' << std::flush;
As you see this forces the buffer to be flushed every time. This is very inefficient. So don't do it every time just do it once you have done all your printing. Personally I hardly ever use it. std::cin and std::cout are synced together so that std::cout is flushed before user input is requested so it is hardly ever necessary and the buffers for files are designed to be the size that gives you the most effecient size for flushing to disk.
cout << tail->test.int1 << " " << tail->test.string1 << endl;
Rewrite the loop as:
for(TestPtr printLoop = head; printLoop != NULL; printLoop = printLoop->next)
std::cout << printLoop->test << '\n';
// Note: it is not TestList job to know how to print a Test
// You should just ask the stream to print it.
// The the Test object should know how to print itself.