0

I have a list with names of employers such as:

Node 1: Jill, Matt, Joe, Bob, Matt

Node 2: Jeff, James, John, Jonathan, John, Edward

Node 3: Matt, Doe, Ron, Pablo, Ron, Chase, Ron, Chase, Loui

and I'm trying to get it to where if it sees a repeat it will send it to the front of the list and delete that current node, so that it will look like this

Node 1: Matt, Jill, Joe, Bob

Node 2: John, Jeff, James, Jonathan, Edward

Node 3: Chase, Ron, Matt, Doe, Pablo, Loui

Unfortunately, My output is close to what I would like. It's deleting the duplicate entries, but it's not sending to the front. .

My output:

Node 1: Jill, Matt, Joe, Bob,

6
  • 2
    The correct implementation of a linked list is std::list Commented Sep 23, 2013 at 0:11
  • Anyone know the correct way to insert to the front of the list? Commented Sep 23, 2013 at 0:31
  • @Mdjon26 - cplusplus.com/reference/list/list/push_front Commented Sep 23, 2013 at 0:48
  • 1
    Are you and Jake Smith taking the same class? Commented Sep 23, 2013 at 0:56
  • Adding to the front of a list is much easier then adding to the end. If you don't need to add the end try adding to the front. Commented Sep 23, 2013 at 0:58

4 Answers 4

1

Well, let's see:

When you hit if (ptr->data == p->data) at that point:

  • pp points to the end of the list
  • p is you new node (nothing points to it and it points to nothing)
  • ptr points to the node with duplicate data

In order to delete the node you need to actually need to have the next pointer pointing to ptr otherwise how can you remove ptr from the list? So you would actually need to check:

if (head && head->data == p->data)
{
    // do nothing as duplicate entry is already head of list
    delete p;
    return;
}

node *ptr = head;
while (ptr)
{
    if (ptr->next && ptr->next->data == p->data)
    {
        node *duplicate = ptr->next;
        ptr->next = duplicate->next; // skip the duplicate node
        duplicate->next = head;      // duplicate points to head
        head = duplicate;            // head is now the duplicate
        delete p;                    // otherwise leaking memory
        return;
    }
    ptr = ptr->next;
}

if (pp) // points to tail as per your code
{
    pp->next = p;
    ++N;
}
Sign up to request clarification or add additional context in comments.

Comments

0

I changed your variable names to be more readable, but kept the forward-declaration in case you wanted it like that. The issues I found were that you were always inserting the node at the end of the list, regardless of if you found it was a duplicate. In addition, your commented lines appeared close, but only if alone. With the pp=p stuff, it would cause issues. Try the following and see if it works. You'll still be leaking memory, but it will get you started:

void list::put(int i) {  //Where i is a random number
    node *current =head;
    node *added =new node(employers[i]);
    node *tail =head;
    node *prev = NULL;
    bool foundRepeat = false;



    while (current!=NULL)
    {
        if (current->data == added->data)
        {  
            if (prev)
                prev->next = current->next;

            current->next=head;
            head=current;
            foundRepeat = true;
            break;
        }
        prev = current;
        current=current->next;
    }

    if (!foundRepeat)
    {
        while (tail->next) 
        {
            tail=tail->next;
        }
        tail->next=added;
    }

    N++;
}

2 Comments

Try this with A->B->C and now add B. You will end up with a circle from A to B back to A and have lost C. Not to mentioned the memory leak.
yeah, I wasn't concerned about the memory leak. He should be handling that/learning it as he goes, though I probably should have mentioned it. And you're right about the loop, updated and fixed...
0

For what it's worth, I'd probably implement it like this.

class EmployerCollection
{
public:
    typedef std::list<std::string> EmployerList;

public:
    bool AddEmployer(const std::string& name)
    {
        EmployerList::const_iterator it = std::find(m_employers.begin(), m_employers.end(), name);
        if (it != m_employers.end()) // Already exists in list.
        {
            m_employers.splice(m_employers.begin(), m_employers, it, std::next(it));
            return true;
        }
        m_employers.push_front(name);
        return false;
    }

private:
    EmployerList m_employers;
};

int main()
{
    const int NUM_EMPLOYERS = 15;
    std::string employers[NUM_EMPLOYERS] = {"Jill", "Jeff", "Doe", "Pablo", "Loui", "Ron", "Bob", "Joe", "Monica", "Luis", "Edward", "Matt", "James", "Edward", "John"};

    EmployerCollection c;

    for (int i=0; i<NUM_EMPLOYERS; i++)
    {
        bool duplicate = c.AddEmployer(employers[i]);
        printf("Added %s to employer list - duplicate: %s \n", employers[i].c_str(), duplicate ? "True" : "False");
    }

    system("pause");
} 

2 Comments

Best not to use a constant to define the size of the array NUM_EMPLOYERS. Prefer to let the compiler calculate it. then you can get the size of NUM_EMPLOYERS. Prefer: std::string employers[] = { STUFF }; Then const int NUM_EMPLOYERS = sizeof(employers)/sizeof(employers[0]);
I don't see the difference really, it was used purely for the sake of the example, and having a variable to use during the loop. The fact that there is an array of hand written strings there in the first place defeats the purpose of the whole exercise.
0

I've added a find function

typedef struct node{
  string data;
  struct node *net, *prev;
 }node;      


class list {
public:
    list():head(NULL), N(0){}
    ~list(){
    //Implementation for cleanup
     }

void add(string name){  //rather than accessing the global data, use the value passed
    node* p = new node(name);
    p->next=p->prev=NULL;
    node* pp = find(name);
    if(pp==NULL){
      // No match found, append to rear
      if(head==NULL)
        head=p;  //list empty, add first element
      else{
        node* cur=head;
        while(cur->next!=NULL) //Keep looking until a slot is found
          cur=cur->next;
        cur->next=p;
        p->prev=cur;
      }
    }
    else{
        //Match found, detach it from its location
        node* pPrev = pp->prev;
        pPrev->next = pp->next;
        pp->next->prev=pPrev;
        p->next = head; //append it to the front & adjust pointers
        head->prev=p;
    }
    N++;
    }

    //MER: finds a matching element and returns the node otherwise returns NULL
    node* find(string name){
        node *cur=head;
        if(cur==NULL) // is it a blank list?
          return NULL;
        else if(cur->data==head) //is first element the same?
          return head;
        else   // Keep looking until the list ends
          while(cur->next!=NULL){
          if(cur->data==name)
            return cur;
            cur=cur->next;
          }
        return NULL;
}
friend ostream& operator << (ostream& os, const list& mylist);

private:
    int N;
    node *head;

};

Now some may tell you to use the list in STL n never to write your own code coz you can't beat STL, but to me it's good that you are implementing your own to get a clear idea on how it works in reality.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.