Dobubly Linked List Adding Default Entries Upon Execution

[cpJOIJOIJLKM]

Hi, I have a program to model a small music database. It uses a doubly linked list to model the 'SongList', but my issue is that it seems to add two nodes of data from the default constructor every time it runs. I'm not sure where the problem is, but I'm beginning to believe it must lie within my addSong function, as this is initializes the list.

So to clarify the problem looks like this, say I have my music data like:

Title12;Artist1;1;1;Album1
Title2;Artist1;1;1;Album1
Title3;Artist1;1;1;Album1
Title4;Artist1;1;1;Album1
Title5;Artist1;1;1;Album1
Title6;Artist1;1;1;Album1
Title7;Artist1;1;1;Album1
Title8;Artist1;1;1;Album1
oijcoiacj;049;24;24;dposk
totot;d;3;2;1

After running the program - and adding no new songs - it will display my SongList (and save it to the file) as:

No title;No artist;0;0;No album
No title;No artist;0;0;No album
3;3;3;3;3
Title1;Artist1;1;1;Album1
Title10;Artist1;1;1;Album1
Title11;Artist1;1;1;Album1
Title12;Artist1;1;1;Album1
Title2;Artist1;1;1;Album1
Title3;Artist1;1;1;Album1
Title4;Artist1;1;1;Album1
Title5;Artist1;1;1;Album1
Title6;Artist1;1;1;Album1
Title7;Artist1;1;1;Album1

And I cannot figure out why this is happening. I've tried modifying the displayList and writeFile functions, but I believe it is happening at initialization, perhaps within the addSong function or loadFile. The existing code needs a lot of work, but for now I'm just focusing on this bug. I believe the rest should be easy enough to complete after I solve this.

The complete code is here:
https://wandbox.org/permlink/g3xgy54NiOoOWSZ9

But the relevant file is below:

//This is the implementation file for SongList class
#include "songlist.h"
#include "tools.h"

//SongList implementation functions
//default constructor
SongList::SongList()
{
  size = 0;
  head->prev = nullptr;
  head->next = tail;
  tail->prev = head;
  tail->next = nullptr;
  Node * next = NULL;
  Node * prev = NULL;
}
//constructor from file
SongList::SongList(const char fileName[])
{
	ifstream inFile;
	Song aSong;
	char tempTitle[MAXCHAR], tempArtist[MAXCHAR], tempAlbum[MAXCHAR];
	int tempMin = 5;
	int tempSec = 5;
	inFile.open(fileName);
        head = new Node();
	tail = new Node();
	head->prev = nullptr;
	head->next = tail;
	tail->prev = head;
	tail->next = nullptr;
	
	if (!inFile)
	{
		cout << "File not found! Program terminating!!" << endl;
		exit(0);
	}
	size = 0;
	inFile.get(tempTitle, MAXCHAR, ';');
	while (!inFile.eof())
	{
	       	inFile.ignore(5, ';');
		inFile.get(tempArtist, MAXCHAR, ';');
		inFile.ignore(5, ';');
		inFile >> tempMin;
		inFile.ignore(5, ';');
		inFile >> tempSec;
		inFile.ignore(5, ';');
		inFile.get(tempAlbum, MAXCHAR, '\n');
		inFile.ignore(5, '\n');
		// populate aSong with all the information
		aSong.setTitle(tempTitle);
		aSong.setArtist(tempArtist);
		aSong.setMin(tempMin);
		aSong.setSec(tempSec);
		aSong.setAlbum(tempAlbum);
		this->addSong(aSong);
		inFile.get(tempTitle, MAXCHAR, ';');
	}
	inFile.close();
}

//destructor
SongList::~SongList()
{
  //  delete[] list;
}

bool SongList::addSong(Song& aSong)
{
  Node * newTail = new Node;
  newTail->data = aSong;
  newTail->prev = tail;
  if(tail){
    tail->next = newTail;
  }
  tail = newTail;
  size++;
  return true;
}

// handle edge cases when current = head or current = tail
void SongList::delSong()
{
  int selection = 0;
  displayList();
  Node * current = head;
  cout << "Which song would you like to delete?: ";
  cin >> selection;
  while (!cin or selection <= 0){
    cout << "Invalid selection. Try again: ";
    cin.clear();
    cin.ignore(100, '\n');
    cin >> selection;
  }
  for (int i = 0; i < selection; i++){ 
    current = current->next;
  }
  Node * prevNode = current->prev;
  prevNode->next = current->next;
  current->prev = prevNode->next;
  delete current;
  size--;
  displayList();
}

//prints the list
void const SongList::displayList()
{
  Node * current = head;
  while (current != nullptr)
    {
      current->data.printSong();
      current = current->next;
    }
}

//finds a song in the list
void const SongList::findSong()
{
	char srchTitle[MAXCHAR], tempTitle[MAXCHAR];
	cout << "Enter the song  you are looking for:";
	cin.get(srchTitle, MAXCHAR, '\n');
	convertCase(srchTitle);
	Node * current = head;
	while (current != nullptr)
	  {
	    current->data.getTitle(tempTitle);
	    convertCase(tempTitle);
	    if (strcmp(tempTitle, srchTitle) == 0)
	      {
	      }
	  }
}


//copy constructor for song class?
//writes the data back to the file
void SongList::writeFile()
{
	ofstream outFile;
	Node * current = head;
	outFile.open("songs.txt");
	if (!outFile)
	{
		cout << "File not found! Program terminating!!" << endl;
		exit(0);
	}
	
	while (current != nullptr)
	{
	  current->data.printFile(outFile);
	  current = current->next;
	}
	outFile.close();
}

Thanks for stopping by!

[ne555]

in your default constructor `head' and `tail' are invalid, you cannot dereference them
in the other constructor you have

//SongList::SongList(const char fileName[])
	head = new Node();
	tail = new Node();
	head->prev = nullptr;
	head->next = tail;
	tail->prev = head;
	tail->next = nullptr;

there you already have two nodes on your list
if you don't want those extra nodes, then don't create them.


alternatively, if it makes the rest of the functions easier, you may simply ignore them

[cpJOIJOIJLKM]

So in my default constructor, should I remove all lines that mention head and/or tail?

If I don't create the extra two nodes in the other constructor, I experience a segfault. But you are right, that is where the problem lies. I am not quite sure how to properly initialize my pointers in this instance though.

Sorry, I'm a little overwhelmed and perhaps in over my head here. Could you show me what a proper initialization might look like?

[ne555]

If you want to do it linear and with `head' and `tail' pointers, then you need to start at null, and update them in the insert operation

class list{
   Node *head, *tail;
};
list::list():
   head(nullptr),
   tail(nullptr)
{}

void list::push_back(Element e){
   if (not head){
      tail = head = new Node(e, nullptr, nullptr);
      return;
   }

   tail->next = new Node(e, tail, nullptr);
   tail = tail->next;
}

void list::display_list() const{
   // what you have in your code
}

I prefer a circular list with a header cell. This header cell does not hold relevant data, it's just there so all the pointers are always valid.

class list{
   Node head; //note: not a pointer
   //note: no need for tail
};

list::list()
{
   head.next = head.prev = &head; //circular
}

void list::display_list() const{
   Node *current = &head;
   Node *tail = head.prev; //easy to get the last one
   while (current not_eq tail){ //tail marks the end of the list
      std::cout << current->next->data << ' '; //the data is on the `->next'
      current = current->next;
   }
}

void list::push_back(Element e){
   Node *tail = head.prev;
   // T: last element, H: head
   // ... <-> T <-> H <-> ...

   head.prev = new Node(e, tail, &head); //the Node constructor initialise data=e, prev=tail, next=&head
   // insert N: new element, now we have
   // ... <-> T -> H
   // T <- N <-> H

   tail->next = head.prev;
   // so we fix T->next to close the list
   // ... <-> T <-> N <-> H
}

In either case, your SongList::SongList(const char fileName[]) should be just construct a list followed by push_back() operations