C++: STL linked list - += duplicating node

I am working on a Polynomial class which uses the STL linked list. One of the functions requires me to add two Polynomial's together. For some reason, the += operator seems to be duplicating the node, as opposed to merely modifying the contents.

Here is the class declaration:

class Polynomial
{
    public:

        Polynomial(pair<double,int>); //Specified constructor
        void add(const Polynomial&);
        void print();
    private:

        Polynomial(); //Default constructor
        list<pair<double,int> > terms;
};

This is the add member function:

void Polynomial::add(const Polynomial& rhs)
    {
    list<pair<double,int> >::const_iterator r;
    list<pair<double,int> >::iterator l;

    for(r=rhs.terms.begin(); r!=rhs.terms.end(); r++)
    {
        bool match=0;
        //Check to see if we have an existing nth order node
        for(l=terms.begin(); l!=terms.end(); l++)
        {
            //If we do, just add the coefficients together
            if(l->second == r->second)
            {
                l->first += r->first;
                match = 1;


            }       
        }

        //If there was no matching existing node, we need to find out
        //where to insert it into the list.
        if(!match)
        {
            l=terms.begin();
            bool inserted=0; //Sentinel for the loop

            while(l!=terms.end() && !inserted)
            {               
                //If there's only one term in the list
                //Just compare and stick it in front or behind the existing node
                if(terms.size()==1)
                {
                    int this_exp = l->second;
                    int exp_to_ins = r->second;
                    if(exp_to_ins > this_exp) terms.push_back((*r));
                    if(exp_to_ins < this_exp) terms.push_front((*r));
                    inserted = 1;

                }

                //If there's more than one node, we need to traverse the list
                if(terms.size()>1)
                {
                    if(l!=terms.begin())
                    {
                        int this_exp = l->second;
                        l++;
                        int next_exp = l->second;
                        int exp_to_ins = r->second;

                        //If the new node value is between the current and next node
                        //Insert between them.
                        if((this_exp < exp_to_ins) && (exp_to_ins < next_exp))
                        {
                            terms.insert(l,(*r));                    
                            inserted = 1;
                        }

                     } 
                     else if(l==terms.begin())
                     {
                        int this_exp = l->second;
                        int exp_to_ins = r->second;


                        //This will be the smallest order node
                        //Put it in the top spot
                        if(this_exp > exp_to_ins) 
                        {
                            terms.push_front((*r));
                            inserted = 1;
                        }

                        l++;                    
                     }                  

                }

            }

            //If we've traversed the list and can't find the right place
            //this must be the greatest order node in the list
            //so just tack it on开发者_如何学Go the end.
            if(!inserted) terms.push_back((*r));

        }

    }


}

Works fine with ordering the nodes in the correct order, but we have an existing nth order node, rather than just adding the coefficients together, it keeps the original node but seems to make a second node with the coefficients added together, and I have no idea why.

If I run the print function, for what should be F(x) = -2x^7 + 3x^6 - 11x^5 - 2x^4, instead I get F(x) = -2x^7 + 3x^6 - 11x^5 - 10x^5. If I call the size() function on the list, I get 4. But if I run the following code to print out the info from the nodes in the list:

stringstream test;   
for(i=terms.end(); i!=terms.begin(); i--)
{

    test << "Coefficient: " << i->first << " ";
    test << "Exp: " << i->second << endl;

}
cout << "Size: " << terms.size() << endl;
cout << test.str(); 

The following is output:

Coefficient: -10 Exp: 5 Coefficient: -2 Exp: 7 Coefficient: 3 Exp: 6 Coefficient: -11 Exp: 5

Any help greatly appreciated.

EDIT: This is the test program.

Polynomial p(pair<double, int>(-10, 5));
p.add(Polynomial(pair<double,int> (-2,4)));
p.add(Polynomial(pair<double,int> (3,6)));
p.add(Polynomial(pair<double,int> (-2,7)));
p.add(Polynomial(pair<double, int> (-1,5)));

Your add() function seems to be correct except the print:

for(i=terms.end(); i!=terms.begin(); i--)
{    
    test << "Coefficient: " << i->first << " ";
    test << "Exp: " << i->second << endl;
}

This is completely wrong, and invokes undefined behavior. i is initially terms.end() and you've dereferencing it? items.end() returns past-the-end iterator. Even if I assume it correct for a while, the condition i!=terms.begin() means the first element is never printed!

So the fix is this:

for(list<pair<double,int> >::iterator i=terms.begin(); i!=terms.end(); i++)
{

    test << "Coefficient: " << i->first << " ";
    test << "Exp: " << i->second << endl;
}

And it prints expected output:

Size: 4
Coefficient: -2 Exp: 4
Coefficient: -11 Exp: 5
Coefficient: 3 Exp: 6
Coefficient: -2 Exp: 7

Is it not correct?

See the output yourself here also : http://www.ideone.com/p8mwJ

By the way, instead of add, you could make it operator+= instead, as:

const Polynomial& operator+=(const Polynomial& rhs)
{
    //same code as before
    return *this;
}

If you write so, then you can add polynomials as:

Polynomial p(pair<double, int>(-10, 5));
p += Polynomial(pair<double,int> (-2,4));
p += Polynomial(pair<double,int> (3,6));
p += Polynomial(pair<double,int> (-2,7));
p += Polynomial(pair<double, int> (-1,5));

Demo : http://www.ideone.com/aA1zF

I just read your comment, and came to know that you want to print it in reverse order, in that case, you could use rbegin() and rend() instead of begin() and end() as:

for(list<pair<double,int> >::const_reverse_iterator i=terms.rbegin(); 
                                                    i!=terms.rend(); 
                                                    i++)
{

    test << "Coefficient: " << i->first << " ";
    test << "Exp: " << i->second << endl;

}

I would also advice you to make print a const function as :

void print() const
            //^^^^ this makes the function const!

Better yet overload operator<< .

Anyway reverse order printing demo : http://www.ideone.com/Vk6XB

Your test loop (the one printing in the stringstream) is incorrect: it's undefined behavior to dereference the end () iterator. Probably your "std::list" is implemented in a circular way (i.e. with begin == end+1) so dereferencing "end" gives you *begin in your test loop.

Use reverse iterators to print the list in reverse order:

for (i = list.rbegin (); i != list.rend (); ++i)
{
   test << "Coefficient: " << i->first ; // etc.
} 

Besides the problem pointed out by @Nawaz, there is also a problem in the Polynomial::add function.

If the if(terms.size()==1) block is executed, a new item is inserted in the list. But that increases the size of the list by one, so the if(terms.size()>1) block will also be executed. And this can insert the same node once more.

A bit further in the while loop, you increment l, and proceed using the next node, without checking whether it's valid (ie. without comparing to terms.end()).

There might be more such mistakes, but these came up after a cursory glance.