Right now your code does too much. Programs solve a collection of sub-problems in an attempt to solve a larger problem. This leads to the Single Responsibility Principle.
What that means is that one object (class, function, etc.) should solve one problem only. But right now that's not happening. For example, main trivially does more than one thing: it manages nodes for the list (incorrectly, too! Nothing is ever deleted!), and gets input from the user. This is too much.
Rather, split things up. You should make a list class that manages nodes, and then main should use it. Note the difference here: main no longer solves that problem, it utilizes something that does.
So with this in mind, it quickly follows the more we split things up, the easier it is to be correct, fix, and maintain. The act of taking code and splitting it up is "refactoring". Let's do that.
First, we need a linked list to use. Normally we have std::vector (note: linked lists are generally the worse container there is) or std::list, but since your teacher is dumbmisguided, he's making you write your own. Your assignment should be either write a list container or use a list container and read input, not both. (Again, in the real world we split things up; why teach people to mix them?)
You already have the basics down, it just needs to be encapsulated. (If you don't know classes yet, let me know and I'll expand there too; while we're at it, if you don't already you might want to get a good book to teach yourself what your teacher isn't):
// normally this should be a template so it can store anything,
// and yadda yadda (more features), but let's just make it basic
// this data class is what the linked list holds
struct data
{
std::string name;
int id;
};
class linked_list
{
public:
linked_list() :
mHead(0)
{}
// the magic: the destructor will always run
// on objects that aren't dynamically allocated,
// so we're guaranteed our resources will be
// released properly
~linked_list()
{
// walk through the list, free each node
while (mHead)
{
node* toDelete = mHead; // store current head
mHead = mHead->next; // move to next node
delete toDelete; // delete old head
}
}
void push_back(const data& pData)
{
// allocate the new node
node* newNode = new node(pData, mHead);
// insert
mHead = newNode;
}
data pop_back()
{
// remove
node* oldNode = mHead;
mHead = mHead->next;
// deallocate
data d = oldNode->data;
delete oldNode;
return d;
/*
the above is *not* safe. if copying the data throws
an exception, we will leak the node. better would be
to use auto_ptr like this:
// now the node will be deleted when the function ends, always
std::auto_ptr<node> n(oldNode);
// copy and return, or copy and throw; either way is safe
return n->data;
but who knows if your <strike>dumb</strike>misguided
would allow it. so for now, make it unsafe. i doubt
he'd notice anyway.
*/
}
private:
// any class that manages memory (i.e., has a destructor) also needs to
// properly handle copying and assignment.
// this is known as The Rule of Three; for now we just make the class
// noncopyable, so we don't deal with those issues.
linked_list(const linked_list&); // private and not defined means it
linked_list& operator=(const linked_list&); // cannot be copied or assigned
struct node
{
// for convenience, give it a constructor
node(const data& pData, node* pNext) :
d(pData),
next(pNext)
{}
data d; // data we store
node* next; // and the next node
};
node* mHead; // head of list
};
Now you have a list to use. main will no longer be troubled with such things:
#include <cstdlib>
#include <iostream>
#include <fstream>
#include <string>
using namespace std; // should generally be avoided
// your linked_list code
int main()
{
// don't declare variables until you need them,
// and avoid globals. (the previous rule helps)
ifstream sourcefile("A3.txt");
// check that it opened
if (!sourceFile.is_open())
{
cerr << "could not open file" << endl;
// EXIT_FAILURE is inside <cstdlib>
return EXIT_FAILURE;
}
string input;
sourcefile >> input;
cout << "Received command: " << input << endl;
linked_list datalist;
if (input == "%INSERT")
{
string reader;
sourcefile >> reader;
cout << "Insert name: " << reader << endl;
while (reader[0] != '%')
{
data d;
d.name = reader;
sourcefile >> d.id;
datalist.push_back(d);
sourcefile >> reader;
cout << "reader: " << reader << endl;
}
}
}
Note how much easier it is to read. You no longer manage a list, but simply use it. And the list manages itself, so you never leak anything.
This is the route you'll want to take: wrap things into working objects that solve one problem correctly, and use them together.