5
\$\begingroup\$

I know this works but I want to improve this code! I don't expect anyone to re-write this but if you have some "try this instead" then great!

import java.util.*;

public class Polynomial implements Iterable<Object>, Comparable<Polynomial> {
    Comparator<Polynomial> compare;
    private Term head = null;
    //A constructor that accepts a string that defines one polynomial in the same format 
    //as provided in the input file.
    public Polynomial(String file) {
        Scanner scan = new Scanner(file);
        try{
            while(scan.hasNext()){
                addTerm(scan.nextDouble(), scan.nextInt());
            }
        } 
        catch (Exception ex){
            System.out.println(ex.getLocalizedMessage());
            throw new InvalidPolynomialSyntax("Error: Invalid Polynomial Syntax, check input");
        }
    }

    public void addTerm(double coefficient, int exponent ){
        if (exponent < 0){
            throw new InvalidPolynomialSyntax("Error: Invalid Polynomial Syntax, Negative exponents, check input");
        }
        Term current = head;
        if(current == null){ 
            head = new Term(coefficient, exponent);
            head.next = null;
        } 
        else { 
            while(current.next != null){
                current = current.next;
            }
            current.next = new Term(coefficient, exponent);
        }

    }

    /*
     * A compareTo method that compares two polynomials.  If their highest exponents are the 
     * same, their coefficients are compared. If two polynomials have the same highest order exponent with the
     * same coefficients the next highest exponent is examined, and so on.
     */
    @Override
    public int compareTo(Polynomial otherPoly) {
        Term thisCurrent = this.head;
        Term otherCurrent = otherPoly.head;

        while (thisCurrent != null && otherCurrent != null){
            //If the two polynomials have different highest 
            //order exponents, the one with the highest exponent is the greatest.
            if (thisCurrent.getExponent() != otherCurrent.getExponent()){
                return thisCurrent.getExponent() - otherCurrent.getExponent();
            }
            else if(thisCurrent.getCoefficient() != otherCurrent.getCoefficient()) {
                if(otherCurrent.getCoefficient()> thisCurrent.getCoefficient()){
                    return -1;
                }
                else if(otherCurrent.getCoefficient()< thisCurrent.getCoefficient()){
                    return +1;
                }
            }
            thisCurrent = thisCurrent.getNext();
            otherCurrent = otherCurrent.getNext();
        }//if both are null they are equal
        if (thisCurrent == null && otherCurrent == null){
            return 0;
        }//If two polynomials have the same highest order exponent with the
         //same coefficients the next highest exponent is examined, and so on.
        if (thisCurrent == null){
            return -1;
        }else {
            return +1;
        }
    }
    //If the exponents are the same...
    public int compareExponents(Polynomial poly2) {
        Term thisPolyTerm = this.head;
        Term otherPolyTerm = poly2.head;
        while(thisPolyTerm != null && otherPolyTerm != null) {
            if (thisPolyTerm.getExponent() != otherPolyTerm.getExponent()) {
                return thisPolyTerm.getExponent() - otherPolyTerm.getExponent();
            }
            else {
                thisPolyTerm = thisPolyTerm.getNext();
                otherPolyTerm = otherPolyTerm.getNext();
            }
        }
        if(thisPolyTerm == null && otherPolyTerm == null){
            return 0;
        }
        if (otherPolyTerm == null){
            return +1;
        }
        else {
            return -1;
        }
    }

    public Polynomial() { 
        compare = (Polynomial poly1, Polynomial poly2) -> poly1.compareExponents(poly2); 
        }
    public Polynomial(Comparator<Polynomial> compare){ 
        this.compare = compare; 
        }

    @Override
    public Iterator<Object> iterator() {
        return new Iterator() {

            private Term current = getHead();

            @Override
            public boolean hasNext() {
                return current != null && current.getNext() != null;
            }

            @Override
            public Term next() {
                Term data = current;
                current = current.next;
                return data;
            }
        };
    }

    @Override
    public String toString() {
        StringBuilder expressionBuilder = new StringBuilder();
        //first check head to avoid +1x^3 +3x^2
        if (head.coefficient > 0){
            expressionBuilder.append(head.toString());
        }
        else {
            expressionBuilder.append(" - ").append(head.toString());
        }
        for(Term tmp = head.next; tmp != null; tmp = tmp.next) {
            if (tmp.coefficient < 0) {
                expressionBuilder.append(" - ").append(tmp.toString());
            } 
            else {
                expressionBuilder.append(" + ").append(tmp.toString());
            }
        }
        return expressionBuilder.toString();

    }

    static class Term{
        private double coefficient;
        private int exponent;
        private Term next;

        private Term(double c, int e) {
            coefficient = c;
            exponent = e;
            next = null;
        }

        private int getExponent(){
            return this.exponent;
        }
        private double getCoefficient(){
            return this.coefficient;
        }
        private Term getNext(){
            return next;
        }

        @Override
        public String toString(){
            String termString = String.format("%.1f", Math.abs(coefficient));
            if (exponent == 0) { 
                return termString;
            }
            else if(exponent == 1){ 
                return termString + "x";
            } 
            else{
                return termString + "x^" + exponent;
            }
        }
    }

    private Term getHead() {
        return head;
    }
}

I don't think this part needs anything changed to it but open to suggestions(mainly providing this so the code compiles and runs)

OrderedList.Java

import java.util.*;

/*
 * The third class is OrderedList, which is a utility class 
 * that contains two overloaded implementations
 */
public class OrderedList {
    //Both methods should be class (static) methods.
    //The first of the overloaded methods should accept a list 
    //that contains elements that implement Comparable.
    public static <T extends Comparable<? super T>> 
    //two overloaded implementations of a method named checkSorted
    boolean checkSorted(List<T> list){
        boolean isSorted = true;

        for(int i = list.size()-1; i > 0 ; i--){
            T current = list.get(i);
            if(!checkSorted(list, current)){
                isSorted = false;
            }
        }
        return isSorted;
    }

    //The second should instead be supplied an additional 
    //parameter that is an object of a class that implements 
    //the Comparator interface
    private static <T extends Comparable<? super T>> 
            boolean checkSorted(List<T> list, T current) {

        
        T currentValue = list.get(list.indexOf(current));
        T nextValue = list.get(list.indexOf(current) - 1);

        if (nextValue != null) {

            return currentValue.compareTo(nextValue) >= 0; 
        }
        return true;
    }
}

Please ignore my notes, ha ha Main.java

import javax.swing.*;
import java.io.*;
import java.util.*;

public class Main {

    private static List<Polynomial> polyList = new ArrayList<>();

    public static void main(String[] args) {
        processPolyList();
    }

    public static ArrayList<String> fromFile() {
        //Create ArrayList and JFileChooser
        ArrayList<String> expressionList = new ArrayList<>();
        JFileChooser chooser = new JFileChooser();
        //Show directories and files
        chooser.setFileSelectionMode(JFileChooser.FILES_AND_DIRECTORIES);
        //user's current directory
        chooser.setCurrentDirectory(new File(System.getProperty("user.dir")));
        int response = chooser.showOpenDialog(null);
        if (response == JFileChooser.APPROVE_OPTION){
            File file = chooser.getSelectedFile();
            try {
                Scanner scan = new Scanner(file);
                if (file.isFile()){
                    while (scan.hasNextLine()){
                        String expression = scan.nextLine();
                        expressionList.add(expression);
                    }
                }
            }
            catch (NoSuchElementException a){
                JOptionPane.showMessageDialog(JOptionPane.getRootFrame(),"File is empty!");
            }
            catch(FileNotFoundException b){
                JOptionPane.showMessageDialog(JOptionPane.getRootFrame(),"404 File Not Found!");
            }
        }
        return expressionList;
    }

    public static boolean checkWeakOrder( List<Polynomial> polyList){
        boolean isWeakOrder = true;
        Polynomial previous = polyList.get(polyList.size()-1);
        for(int i = polyList.size()-2; i > 0; i--){

            if (previous.compareExponents(polyList.get(i)) < 0){
                isWeakOrder = false;
            }
        }
        return isWeakOrder;
    }

    public static void processPolyList(){
        try {
            ArrayList<String> a = fromFile();
            for (String element : a) {
                Polynomial p = new Polynomial(element);
                System.out.println(p);
                polyList.add(p);
            }
        }
        catch (InvalidPolynomialSyntax ex){
            JOptionPane.showMessageDialog(JOptionPane.getRootFrame(),ex.getMessage());
        }

        System.out.println("This list is sorted by the strong order: " + OrderedList.checkSorted(polyList));
        System.out.println("This list is sorted by the weak order: " + checkWeakOrder(polyList));
    }
}

InvalidPolynomialSyntax.java


/*
 * The second class is InvalidPolynomialSyntax, which defines an 
 * unchecked exception that contains a constructor that allows a 
 * message to be supplied. It is thrown by the constructor of 
 * the Polynomial class should the supplied string contain 
 * coefficients or exponents of an improper type or should the 
 * exponents fail to be listed in strictly descending order.
 */
public class InvalidPolynomialSyntax extends RuntimeException {
    
    InvalidPolynomialSyntax(String message){
        super(message);
    }
}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

I'm only looking at Polynomial and your Exception class.

General

Classes not carefully and intentionally designed to be extended should be marked as final.

All of your classes are missing meaningful javadoc.

Comments that explain what code is doing generally indicates that the code is too complex. Readers should be able to easily tell what the code is doing by reading it. Comments should be used to explain why code is doing what it's doing, and only when the reasons are not obvious. This should be rare.

In java, catch and else are typically on the same line as }.

In java, there is whitespace before a {.

In java, there is whitespace between control flow words (if, for, etc) and the open paren. This visible distinguishes them from method invocations, where there is no space.

Polynomial

compare is a verb, indicating an action, but it is an object, which is a thing. comparator or some other noun would be better.

compare should not be visible outside Polynomial, and should be private.

compare is never assigned outside a constructor. It should be marked final.

The first constructor never assigns a value to compare. In fact, the value is never used. Perhaps the comparator should be used if it exists, otherwise your default should be used?

The first constructor reads from a file to build the Polynomial. That's a very inflexible approach. Why does a Polynomial inherently know how it's stored? Let clients load the file.

Scanner should be used in a try-with-resources block.

There is no compelling reason for Term to know it's part of a linked list. Terms are not inherently connected. The concept of a Term is separate from the implementation detail that they're being stored in a List. Prefer using a provided List implementation, such as LinkedList, to hold a collection of Terms. Using a List implementation will allow significant cleanup of the code.

Term is probably a big enough idea to have its own class.

toString for a term is always suppressing the + or -. That's not correct. A negative Term should not think it renders without a -. Better choices would be to either always suppress only a + sign, or to never suppress either and let the Polynomial kill the leading +. It comes down to whether a Term wants to render the + when it is displayed by itself. If I System.out.println(new Term(3.0, 2);, is that +3x^2 or 3x^2? I think the latter.

You don't need else blocks when you're returning from Term.toString(). The return interrupts the flow of the method.

Term does not check for a 0 coefficient, which is not valid.

c and e are poor variable names in Term. Prefer readable variable names.

Term's variables can be marked final, as they do not change.

If you find through experimentation that Term.toString() is called frequently and is a performance bottleneck, you can precompute the string representation in the constructor and store it in a variable. Since Term is immutable, that representation will always be valid. This is somewhat less readable, so don't do it until you have a tested, proven performance issue around this method.

You have a private getHead method and a private head variable. The method is extraneous.

It is unclear why iterator returns an Iterator<Object> instead of an Iterator<Term>, but that's probably wrong.

All the constructors should be co-located and at the top of the class definition.

"Error: Invalid Polynomial Syntax" isn't telling us anything the class name doesn't already tell us. Including the erroneous input would help for debugging.

The check for exponent < 0 belongs in the constructor for Term, because it's an invariant of the Term class.

Note that you're not handling duplicate exponents at all. Is that an exception? Should the coefficients be added together? Whichever you choose, you should include that in the javadoc for this method so callers are not surprised.

You should decide if Polynomials can be compared in multiple ways or only one way. implements Comparable should be used only if Polynomials have a natural ordering. Read the javadoc for comparable carefully to make this determination. Note that you should strongly consider implementing equals (and therefore hashCode) if you decide to implement Comparable. If they do have a natural ordering, I don't think it makes sense to allow clients to specify a different ordering. If you want clients to be able to specify an ordering of Polynomials, don't implement Comparable and instead use Comparators to sort them.

compareExponents also isn't wired into anything and doesn't make any sense.

I would argue that Polynomials do have a natural ordering, and should implement Comparable. I'd say that Terms also have a natural ordering. Making Terms comparable will simplify the comparison logic.

toString is also doing too much work in Polynomial. A term should know how to render itself.

If you made all these modifications, your code might look something like this.

PolynomialSyntaxException:

public final class PolynomialSyntaxException extends RuntimeException {
    PolynomialSyntaxException(String message){
        super(message);
    }
}

Polynomial:

import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.ListIterator;
import java.util.Objects;

public final class Polynomial implements Iterable<Term>, Comparable<Polynomial> {

    private final List<Term> terms = new LinkedList<>();

    public Polynomial() {
        super();
    }

    public void addTerm(Term term) {
        ListIterator<Term> iterator = terms.listIterator();
        while (iterator.hasNext()) {
            Term currentTerm = iterator.next();
            if (term.getExponent() == currentTerm.getExponent()) {
                throw new PolynomialSyntaxException(
                        "Duplicate exponent found between " + this.toString() + " and " + term);
                // TODO: Or add them together?
            }

            if (term.getExponent() > currentTerm.getExponent()) {
                iterator.add(term);
                return;
            }
        }
        terms.add(term);
    }

    @Override
    public int compareTo(Polynomial polynomial) {
        Iterator<Term> thisTerms = this.terms.iterator();
        Iterator<Term> otherTerms = polynomial.terms.iterator();
        while (thisTerms.hasNext() && otherTerms.hasNext()) {
            Term thisTerm = thisTerms.next();
            Term otherTerm = otherTerms.next();
            int result = thisTerm.compareTo(otherTerm);
            if (result != 0) {
                return result;
            }
        }

        if (thisTerms.hasNext()) {
            return 1;
        }

        if (otherTerms.hasNext()) {
            return -1;
        }

        return 0;
    }

    @Override
    public Iterator<Term> iterator() {
        return terms.iterator();
    }

    @Override
    public boolean equals(Object o) {
        if (o == this) {
            return true;
        }
        if (!(o instanceof Polynomial)) {
            return false;
        }
        Polynomial polynomial = (Polynomial) o;
        return this.terms.equals(polynomial.terms);
    }

    @Override
    public int hashCode() {
        return Objects.hash(this.terms);
    }

    @Override
    public String toString() {
        if (this.terms.isEmpty()) {
            return "";
        }

        StringBuilder expressionBuilder = new StringBuilder();
        Iterator<Term> termIterator = this.terms.iterator();
        expressionBuilder.append(termIterator.next());
        while (termIterator.hasNext()) {
            Term term = termIterator.next();
            if (term.getCoefficient() < 0) {
                expressionBuilder.append(" - ");
                expressionBuilder.append(term.toString().substring(1));
            } else {
                expressionBuilder.append(" + ");
                expressionBuilder.append(term);
            }
        }

        return expressionBuilder.toString();
    }

    public static void main(final String[] argv) {
        Polynomial polynomial = new Polynomial();
        polynomial.addTerm(new Term(-3.0, 5));
        polynomial.addTerm(new Term(3.1, 4));
        polynomial.addTerm(new Term(-3.2, 3));
        System.out.println(polynomial);
    }
}

Term:

import java.util.Comparator;
import java.util.Objects;

public final class Term implements Comparable<Term> {
    private static final Comparator<Term> NATURAL_ORDER =
            Comparator.comparingInt(Term::getExponent).thenComparingDouble(Term::getCoefficient);

    private final double coefficient;
    private final int exponent;

    public Term(double coefficient, int exponent) {
        this.coefficient = coefficient;
        this.exponent = exponent;

        if (this.coefficient == 0) {
            throw new PolynomialSyntaxException("Coefficient may not be 0");
        }

        if (this.exponent < 0) {
            throw new PolynomialSyntaxException("Exponent must be positive, was " + exponent);
        }

    }

    public double getCoefficient() {
        return this.coefficient;
    }

    public int getExponent() {
        return this.exponent;
    }


    @Override
    public int compareTo(Term term) {
        return NATURAL_ORDER.compare(this, term);
    }

    @Override
    public boolean equals(Object o) {
        if (o == this) {
            return true;
        }
        if (!(o instanceof Term)) {
            return false;
        }
        Term term = (Term) o;
        return (this.coefficient == term.coefficient) && (this.exponent == term.exponent);
    }

    @Override
    public int hashCode() {
        return Objects.hash(coefficient, exponent);
    }

    @Override
    public String toString() {
        String termString = String.format("%.1f", coefficient);

        if (exponent == 0) {
            return termString;
        }

        if (exponent == 1) {
            return termString + "x";
        }

        return termString + "x^" + exponent;
    }

    public static void main(final String[] argv) {
        System.out.println(new Term(3.1, 2));
        System.out.println(new Term(-3.1, 2));
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Wow..thanks! It is late where I am but I can see some of the major changes you made...I will compare notes in the morning! \$\endgroup\$ Jun 21, 2021 at 9:05
  • \$\begingroup\$ there is still an error under class Term, line Comparator.comparingInt(Term::getExponent).thenComparingDouble(Term::getCoefficient); see this screenshot I was trying to edit it but not succesful. Could you help please? \$\endgroup\$
    – Maja023
    Sep 9, 2022 at 18:19
1
\$\begingroup\$

I'd say the way compareTo behaves is surprising. Consider the polynomials (x + 1) and (0x² + x + 1). Mathematically, they are the exact same, so I would expect them to be treated as equal, yet they are not. So, while (-2x² + x + 1) < (0x² + x + 1) is as expected, I would also expect (0x² + x + 1) == (x + 1), and thus (-2x² + x + 1) < (x + 1). But instead, we have (x + 1) < (-2x² + x + 1) < (0x² + x + 1), which I find unintuitive

Additionally, unless we want the ability to have multiple Terms with the same exponent, I don't think this is the most natural representation of a polynomial. I think it'd feel more natural to have something akin to a mapping from exponent to coefficient. A Map<Integer, Double> (more specifially a NavigableMap<Integer, Double>) could be a good fit for that. This could save us from having to worry about keeping the list sorted, could easily let us detect (and hopefully handle) duplicate exponents, could make it easier to look up the coefficient tied to a certain exponent, etc. And since we'd be working in the neighborhood of Map.Entry<Integer, Double>, the Polynomal.Term class would become unnecessary and could be removed

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.