-1

I'm writing a program that implements sparse polynomials, with two global structs defined as follows:

//Global structs to represent a polynomial
struct Term {
   int coeff;
   int degree;
};

struct Node {
   Term *term;
   Node *next;
};

the class has a private Node *poly;

I am overloading the * operator to get the product of two polynomials.

The header file for this includes the definition:

//Returns a polynomial that is the product of polynomial a and b
friend const Polynomial operator *(const Polynomial &a, const Polynomial &b);

and for the function, I iterate through each linked list that represents a polynomial (a & b) until the nullptr is reached, adding the degrees, and multiplying the coefficients, creating a new term, inserting it to a temporary polynomial, and using my overloaded addition operator to add the temporary polynomial to the polynomial being returned. I have tested the overloaded addition operator and have not received this problem so I don't think that is causing this issue. The issue is the some of the time I get back the correct answer, in example output, the first run is correct, the other run produces some terms and some addresses:

>PolynomialTester
Enter numbebr of terms of the polynomial: 3
    coefficient degree
Enter term 1:   3 0
Enter term 2:   2 1
Enter term 3:   1 2

3 + 2*x^1 + 1*x^2

Enter numbebr of terms of the polynomial: 3
    coefficient degree
Enter term 1:   3 0
Enter term 2:   2 1
Enter term 3:   1 2

3 + 2*x^1 + 1*x^2
9 + 12*x^1 + 7*x^2 + 6*x^3 + 1*x^4 //Correct
>Exit code: 0    Time: 33.22
>PolynomialTester
Enter numbebr of terms of the polynomial: 3
    coefficient degree
Enter term 1:   3 0
Enter term 2:   2 1
Enter term 3:   1 2

3 + 2*x^1 + 1*x^2

Enter numbebr of terms of the polynomial: 3
    coefficient degree
Enter term 1:   3 0
Enter term 2:   2 1
Enter term 3:   1 2

3 + 2*x^1 + 1*x^2
4109104 + 9 + 12*x^1 + 3*x^2 + 2*x^3 + 1*x^4 + 4108992*x^4108944 + 4109392*x^4109136 //Nope
>Exit code: 0    Time: 19.54

The function I am using for the overloaded * is :

const Polynomial operator *(const Polynomial &a, const Polynomial &b) {
//Computes the sum of two polynomials a + b
//Overloaded + operator

   //The new polynomial that will be returned
   Polynomial polyProduct;

   Polynomial tempPoly;
   //Temporary nodes to process
   Node *tempA = a.poly;
   Node *tempB;

   while(tempA != nullptr) {
      tempB = b.poly;
      while(tempB != nullptr) {

         int degree = tempA->term->degree + tempB->term->degree;
         int coeff = tempA->term->coeff * tempB->term->coeff;
         tempPoly.insert(Polynomial::newTerm(coeff, degree));
         tempB = tempB->next;
      }
      polyProduct = polyProduct + tempPoly;
      tempPoly.deletePoly();
      tempA = tempA->next;
   }

   return polyProduct;
}

PS. deletePoly() goes through the polynomial and deletes first the term, and then deletes the node, until it's a nullptr.

The default constructor:

Term* Polynomial::newTerm(int c, int d) {
//Creates a new term
   Term *newTerm = new Term;
   newTerm->coeff = c;
   newTerm->degree = d;
   return newTerm;
}

Node* Polynomial::cons(Term *t, Node *p) {
//Creates a new node
   Node *newNode = new Node;
   newNode->term = t;
   newNode->next = p;
   return newNode;
}

Polynomial::Polynomial() {
//Creates the zero polynomial
   poly = cons(newTerm(0, 0), nullptr);
}

PolynomialTester:

   int main() {
   Polynomial newPoly;
   Polynomial newPoly2;
   Polynomial newPoly3;

   newPoly.readPoly();
   cout << "\n";
   newPoly.printPoly();

   cout << "\n";

   newPoly2.readPoly();
   cout << "\n";
   newPoly2.printPoly();

   newPoly3 = newPoly * newPoly2;
   newPoly3.printPoly();
   newPoly3.deletePoly();
   newPoly2.deletePoly();
   newPoly.deletePoly();
}

Operator+

const Polynomial operator +(const Polynomial &a, const Polynomial &b) {
//Computes the sum of two polynomials a + b
//Overloaded + operator

   //The new polynomial that will be returned
   Polynomial polySum;
   //Temporary nodes to process
   Node *tempA = a.poly;
   Node *tempB = b.poly;

   //While neither node A or B is equal to the nullptr
   while(tempA != nullptr && tempB != nullptr) {

      int degreeA = tempA->term->degree;
      int degreeB = tempB->term->degree;

      if(degreeA < degreeB) {
         polySum.insert(tempA->term);
         tempA = tempA->next;
      }

      else if(degreeA > degreeB) {
         polySum.insert(tempB->term);
         tempB = tempB->next;
      }

      else {
         int coeff = tempA->term->coeff + tempB->term->coeff;
         int degree = degreeA;
         polySum.insert(Polynomial::newTerm(coeff, degree));
         tempA = tempA->next;
         tempB = tempB->next;
      }
   }
   //Incase one of the polynomials still has remaining terms
   while(tempA != nullptr) {
      polySum.insert(tempA->term);
      tempA = tempA->next;
   }
   //Incase one of the polynomials still has remaining terms
   while(tempB != nullptr) {
      polySum.insert(tempB->term);
      tempB = tempB->next;
   }
   //Removes any zero coeffiecient terms
   //Possibly due to a positive and negative coefficient being added
   polySum.remZeroCoeff();
   return polySum;
}

Copy Constructor:

Polynomial::Polynomial(const Polynomial& oP) {
//Constructs a deep copy of the Polynomial being passed

   poly = nullptr;
   //The list to copy
   Node *toCopy = oP.poly;

   while(toCopy != nullptr) {
      insert(toCopy->term);
      poly->next = toCopy->next;
      toCopy = toCopy->next;
   }
}

Overloaded assignment operator, however this is currently commented out in my code as it only seems to make matters worse

    Polynomial& Polynomial::operator =(const Polynomial &a) {

   //Check to see if it's passing itself
   if(this == &a) {
      return *this;
   }

   //Delete current polynomial  
   deletePoly();

   //The poly to copy
   Node *toCopy = a.poly;

   while(toCopy != nullptr) {
      poly = cons(toCopy->term, poly);
      toCopy = toCopy->next;
   }

   //Return a reference to itself
   return *this;
}

Can anyone help my understand why I'm getting addresses as results some of the time?

coopwatts
  • 670
  • 1
  • 8
  • 31
  • `poly->next = toCopy->next` Huh? What does this do? And still no `operator=`. Do you have it? – n. m. could be an AI Dec 01 '15 at 08:27
  • @Dmitriy Wouldn't that be taken care of because of the two while loops after the initial one? – coopwatts Dec 01 '15 at 08:31
  • @n.m. no overloaded assignment operator – coopwatts Dec 01 '15 at 08:36
  • while(toCopy != nullptr) { insert(toCopy->term); poly->next = toCopy->next; toCopy = toCopy->next; } is here poly.insert(toCopy->term)? – Dmitriy Dec 01 '15 at 08:49
  • You are doing strange things in your copy constructor and assignment. Try `{ insert(toCopy->term); toCopy = toCopy->next; }` in both of them. Your `operator =` will reverse the order of terms and your copy ctor has an inexplicable `poly->next = toCopy->next;`. – n. m. could be an AI Dec 01 '15 at 09:42
  • @n.m. I had originally had tried the code you suggested, but I still got the result: 9 + 9155312*x^9155232 + 10*x^2 + 4*x^3 + 1*x^4 where 12*x^1 is the address of what appears to be the coeff and degree – coopwatts Dec 01 '15 at 09:50
  • And on a second run...: 9 + 9 + 9 + 10*x^2 + 10*x^2 + 4*x^3 + 4*x^3 + 10072800*x^10073056 + 10072800*x^10073056 – coopwatts Dec 01 '15 at 09:52
  • One more question - where do you shift your sumPoly nodes in const Polynomial operator +(const Polynomial &a, const Polynomial &b)? – Dmitriy Dec 01 '15 at 09:57
  • @Dmitriy What do you mean shift? – coopwatts Dec 01 '15 at 09:59
  • polySum.insert(tempA->term); Does this create new Node and shift polySum ptr to the next Node after term insertion? – Dmitriy Dec 01 '15 at 10:02
  • "Overloaded assignment operator, however this is currently commented out in my code as it only seems to make matters worse". You claim you know the rule of three, but you evidently don't understand it. Let me try to explain it again. I'll try to be maximally clear and consise. **YOU NEED THE ASSIGNMENT OPERATOR. END OF STORY.** – n. m. could be an AI Dec 01 '15 at 10:08
  • Once you implement both copy constructor and assignment correctly (and **unit test** them) you can start looking for bugs in `operator*` and whatnot. Until then, there's no point. – n. m. could be an AI Dec 01 '15 at 10:14
  • @Dmitriy Yes, it keeps track of "previous" and "after", once it finds the position then prev->next = cons(term, after); – coopwatts Dec 01 '15 at 10:19
  • I have resolved the issue, and I think I know what the problem was (partially). The line tempPoly.deletePoly(); was causing the issue, so to increase efficiency and also solve the problem, I added a condition in the insert function that checks if the term's degree being passed is equal to a terms degree in the current polynomial, if it is, then just add the coefficients of that term and delete the term after cause it's not needed. This eliminated the need to do any deleting or overloaded adding in the overloaded * function, and increased efficiency. – coopwatts Dec 01 '15 at 22:09

1 Answers1

0

The line tempPoly.deletePoly(); was causing the issue, so to increase efficiency and also solve the problem, I added a condition in the insert function that checks if the term's degree being passed is equal to a terms degree in the current polynomial, if it is, then just add the coefficients of that term and delete the term after cause it's not needed. This eliminated the need to do any deleting or overloaded adding in the overloaded * function, and increased efficiency.

n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243
coopwatts
  • 670
  • 1
  • 8
  • 31
  • Explain how this introduces memory leaks? There is nothing in that answer that introduces a memory leak, in fact the answer eliminates the need for temporary free store memory at all. Delete poly is still being called on the polynomials in the main function but it is not being called in the overloaded multiplication function because there is nothing to delete. Like I said in the answer, if ONE term is created in the insert method that is not needed, it is deleted. In addition to this this destructor, copy constructor and overloaded assignment operator are all in place. – coopwatts Dec 03 '15 at 18:41
  • I apologise, I have misread your explanation. For some reason I imagined you have removed deletePoly() from the destructor. There is no memory leak. Downvote removed. – n. m. could be an AI Dec 03 '15 at 18:49
  • No wait, you don't have deletePoly() in the destructor, right? That's wrong, the destructor is the right place to call it, that's what destructors are designed to do. Anyway if you want to hear constructive critique, post code to codereview. stackexchange.com. – n. m. could be an AI Dec 03 '15 at 18:57