simplifying terms in a polynomial by c++

Question

I am trying to write a function for simplifying terms in a polynomial:

Suppose that we can represent each terms by a two-member int array which the first member shows variable and the second member is the coefficient. for example 4a and 3b could be represented by (1, 4) and (2, 3), respectively. in these examples we have shown the variables (a, b, ..) by an integer based on alphabetic order (a:1, b:2, c:3, ...).

Therefore, a polynomial could be represented as a vector<-int*> of these terms. for example 4a+2b+10c could be shown like {(1,4), (2,2), (3,10)}. Now, the goal is to simplify a polynomial. for example:

2a+3c-a+5t-3c = a+5t
I have written a code in C++ to do this job but it's super slow for large polynomials.

void simplify (vector<int*> &p)
{
    vector<int*>:: iterator it1 = p.begin();
    vector<int*>:: iterator it2;
    while (it1!=p.end())
    {
        if ((*it1)[1]!=0) // if the coefficient is not equal to zero
        {
            it2 = it1+1;
            while (it2!=p.end())
            {
                if ((*it1)[0]==(*it2)[0]) //if the variables are similar
                {
                    (*it1)[1] += (*it2)[1];
                    (*it2)[1]=0; //set the coefficient equal to zero 
                }
                it2++;
            }
        }
        it1++;
    }
    it1 = p.begin();
    while (it1!=p.end()) //removing the terms with zero coefficient
    {
        if ((*it1)[1]==0)
            it1 = p.erase (it1);
        else
            it1++;  
    }
}

I appreciate everyone who can show me what are the code problems and how can I increase my code speed.

Answer 1

I recommend making the code more readable by using a structure for the term:

struct Term
{
  int coefficient;
  char variable_name;
  int  exponent;
};

The above structure models a polynomial term a lot easier than {6,2} would.
Your code becomes more readable, which helps reduce the injection of defects.

A polynomial then becomes:

typedef std::vector<Term> Polynomial;

In words, this reads that a Polynomial is a container (vector) of Terms.

The structure also allows you to sort the container by variable name and descending coefficient (or exponent).

Edit 1: Input of Terms
You can overload the operator>> for the Term to input a term:

struct Term
{
  // as above.
  friend std::istream& operator>>(std::istream& input, Term& t);
};
std::istream& operator>>(std::istream& input, Term& t)
{
  input >> t.coefficient;
  input >> t.variable_name;
  input >> t.exponent;
  return input;
}

This allows you to build a polynomial by:

Polynomial p;
Term t;
while (input >> t)
{
  p.push_back(t);
}

In my opinion, a lot simpler and less error prone than your design and implementation.

Answer 2

There are certainly a few things you may change in your code. But since your chief concern is regarding the efficiency, I would recommend the first action you take to be using a different algorithm.

Your current algorithm checks the remainder of the vector for each element, seeking the same symbol and adding the ones you found to get a final coefficient. Therefore your algorithm is;

for i in range(0, p.size()):
    for j in range(i+1, p.size()):
        // do the processing

As you may notice yourself, the complexity of this algorithm is O(N²), which is the primary reason your code is inefficient.

As an alternative, you may just iterate over the vector and put the values to a std::set. (assuming STL is not off-limits, given your usage of std::vector) Below is a crude implementation of my idea, based on your code.

void simplify (vector<int*> &p)
{
    vector<int*>::iterator it1 = p.begin();
    vector<int*>::iterator it2 = p.end();
    map<int, int> m;
    map<int, int>::iterator itMap1, itMap2;
    while (it1!=it2)
    {
        if(m.find((*it1)[0]) != m.end())
        {
            m[(*it1)[0]] += (*it1)[1];
        }
        else
        {
            m[(*it1)[0]] = (*it1)[1];
        }
        it1++;
    }
    itMap = m.begin();
    itMap2 = m.end();
    p.resize(m.size());
    it = p.begin();
    while (itMap!=itMap2)
    {
        (*it1)[0] = itMap->first;
        (*it1)[1] = itMap->second;
        itMap++;
        it++;
    }
}

The implementation above traverses the vector once, which has O(N) complexity, N being the initial size of the vector. The complexity of the operations conducted in each iteration is O(logN) (due to the implementation of std::map) resulting in the overall complexity of O(NlogN), much better than the initial algorithm with quadratic complexity.

Note that this is only a crude initial implementation based on your code, and there are other possible improvements both for your initial code and for the version I suggest. One example would be using a struct or a class to contain each term as opposed to the pointers, and making your code more readable and more compact. Another possible improvement may be using unordered_map instead of map.