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;
}
Emil Laine
  • 41,598
  • 9
  • 101
  • 157
Vlad
  • 618
  • 2
  • 10
  • 21
  • It's becoming quite a different question after the edit. I don't quite understand, how the loop can terminate now that you never let the iterators become equal to the end. – UncleBens Mar 11 '10 at 15:56
  • I understood why it stalls, it's because it inserts 3x^2 until it will run out of memory eventuallly and crash my computer. Now to see how to fix it. – Vlad Mar 11 '10 at 16:46
  • 1
    Your "last" function doesn't return anything. That might cause a problem. – Mark B Mar 11 '10 at 17:27

3 Answers3

5
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
  • 40,819
  • 6
  • 57
  • 90
  • +1 for the style issues. Better style leads to better programming :) – neuro Mar 11 '10 at 14:29
  • So i should get rid of SetIterBegin()? – Vlad Mar 11 '10 at 14:40
  • Yes, that is unnecessary too. – UncleBens Mar 11 '10 at 14:46
  • Also if you just change the conditional to && the crash may go away, but the polynomial with more terms won't have all its terms in the result. After the main loop you'll need to check both iterators and see if there are remaining terms to put in the result. – Mark B Mar 11 '10 at 14:46
  • @Mark: the crash occurred because of not puting i++ when both equal but you're right i think || is better in yhis case. @UncleBens: but if i pass const Polinom i can't increment the P iterator ans alsi why should i pass &Polinom? – Vlad Mar 11 '10 at 14:52
  • It doesn't compile if i use polinom operator+(const polinom& P) const. – Vlad Mar 11 '10 at 15:07
  • @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 Mar 11 '10 at 15:08
  • 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 Mar 11 '10 at 15:21
  • If you don't change the comparison operator to && then you still allow yourself to access beyond the end of one or the other polynomials, which is likely what's happening. – Mark B Mar 11 '10 at 16:32
  • @Mark see my comment on my post. – Vlad Mar 11 '10 at 16:47
2

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
  • 23,186
  • 6
  • 42
  • 92
1

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
  • 40,819
  • 6
  • 57
  • 90
  • 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 Mar 11 '10 at 16:11
  • 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 Mar 11 '10 at 16:57
  • 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 Mar 11 '10 at 17:19
  • 1
    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 Mar 11 '10 at 17:34
  • I think i understand it now, I'll test it. Thanks again btw! – Vlad Mar 11 '10 at 17:50
  • I supose I'll use const iterators for all operations except division. – Vlad Mar 11 '10 at 18:13
  • 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 Mar 11 '10 at 20:32