views:

395

answers:

3

I'm trying to use operator overloading to define the basic operations (+,-,*,/) for my polynomial class but when i run the program it crashes and my computer frozes.

Update4

Ok. i successfully made three of the operations, the only one left is division.

Here's what I got:

polinom operator*(const polinom& P) const
{
    polinom Result;
    constIter i, j, lastItem = Result.poly.end();
    Iter it1, it2, first, last;
    int nr_matches;

    for (i = poly.begin() ; i != poly.end(); i++) {
         for (j = P.poly.begin(); j != P.poly.end(); j++)
              Result.insert(i->coef * j->coef, i->pow + j->pow);
    }

    Result.poly.sort(SortDescending());

    lastItem--;

    while (true) {
        nr_matches = 0;

        for (it1 = Result.poly.begin(); it1 != lastItem; it1++) {
             first = it1;
             last = it1;
             first++;
             for (it2 = first; it2 != Result.poly.end(); it2++) { 
                  if (it2->pow == it1->pow) {
                      it1->coef += it2->coef;
                      nr_matches++;
                  }
             }

             nr_matches++;
             do {
                last++;
                nr_matches--;
             } while (nr_matches != 0);

             Result.poly.erase(first, last);
        }   
        if (nr_matches == 0)
            break;
    }     

    return Result;
}

Update5

Resolved.

Thanks!

+2  A: 

There are other design and correctness issues with your code, but I think the crash occurs in this line

 if (i->pow > P.i->pow) 

when i == poly.end() && P.i != P.End() or i != poly.end() && P.i == P.End(). Dereferencing i when it points after the last element will crash.

Henrik
+5  A: 
while (i != poly.end() || P.i != P.End())

I think you'll need && there, otherwise the loop terminates only if i and p.i reach their respective end at the same time.

Logic with negations is tricky. Probably simpler to think of this as:

while (!(i == poly.end() || j == P.End())) //while neither iterator has reached end

which according to boolean arithmetic is the same as:

while (!(i == poly.end()) && !(j == P.End()))
while (i != poly.end() && j != P.End())

You also don't seem to be incrementing the iterators if both are equal (infinite loop leading to infinitely many memory allocations?).


Style issues: you are better off using iterators as local variables. Don't make variables class members if they are supposed to be "initialized" before you start using them in a method, and they become useless after the method completes.

Also prefer passing arguments by const reference, and mark member functions const if they don't modify the current object (operator+ shouldn't):

 polinom operator+(const polinom& P) const;

(which would reveal the problem making locally used iterators members - you would be modifying the instances!)

UncleBens
+1 for the style issues. Better style leads to better programming :)
neuro
So i should get rid of SetIterBegin()?
Vlad
Yes, that is unnecessary too.
UncleBens
Mark B
Vlad
Vlad
@Vlad: That's why I said the iterator shouldn't be part of the class. Also, then you should be using `const_iterator`. Passing by const reference is preferable, because it avoids copying the argument. - And yes, the logic of the function may still be flawed, depending on what exactly the result is supposed to be. If you need to handle the remaining items in the longer list, do that separately after the loop that handles both lists at the same time.
UncleBens
I'll post the updated code which works in some cases, please point what I'm doing wrong and where it could be improved (and how).
Vlad
Mark B
@Mark see my comment on my post.
Vlad
+1  A: 

As to getting the function correctly adding up polynomials, I'd recommend this simple logic:

polinom operator+(const polinom& P) const //fixed prototype re. const-correctness
{
    polinom Result;
    std::list<term>::const_iterator //fixed iterator type
        i = poly.begin(), j = P.poly.begin();

    while (i != poly.end() && j != P.poly.end()) {
        //logic while both iterators are valid
    }

    //handle the remaining items in each list
    //note: at least one will be equal to end(), but that loop will simply be skipped

    while (i != poly.end()) {
        Result.insert(i->coef, i->pow);
        ++i;
    }

    while (j != P.poly.end()) {
        Result.insert(j->coef, j->pow);
        ++j;
    }

    return Result;
}

The last part can probably also be left to standard library functions

#include <iterator>
#include <algorithm>

//...
    //handle remaining items in either list (if any)
     std::copy(i, poly.end(), std::back_inserter(Result.poly));
     std::copy(j, P.poly.end(), std::back_inserter(Result.poly));

... but would probably be simpler using list.insert:

     Result.poly.insert(Result.poly.end(), i, poly.end());
     Result.poly.insert(Result.poly.end(), j, P.poly.end());
UncleBens
Using what you just posted it stalls at any input and also if i would inser them like that it wouldn't be ordered from the highest degree to the lowest one.
Vlad
Also what the point of thay empty loop?
Vlad
That empty loop is for **you** to fill in :) (basically the code you already had, without the obfuscation in an attempt to make it do the whole work).
UncleBens
But from what i understand, your code doesn't add terms of the same power and put them in order it simply inserts all of them, am i right?
Vlad
No, the first loop handles both ranges at the same time making sure the order is preserved. It is simpler in that it can safely assume both iterators to be valid. Once you've handled that part, you'll have the last part of either list (or none) unprocessed. You can then just append that part to the result. It will be ordered, since that remaining part should come after the common part. This is a basic merging algorithm.
UncleBens
I think i understand it now, I'll test it. Thanks again btw!
Vlad
Yes it works, sorry for doubting you!
Vlad
I supose I'll use const iterators for all operations except division.
Vlad
You'll (need to) use `const_iterator` when the container is const. Within a const member function, the members of the class, including the `poly` list, are not modifiable. It simply wouldn't compile otherwise. The difference is that a `const_iterator` doesn't allow you to modify the items in the container.
UncleBens
@UncleBens please check my updated question!
Vlad