4
\$\begingroup\$

The following is the web exercise 3.2.12. from the book Computer Science An Interdisciplinary Approach by Sedgewick & Wayne:

Write a program that draws a vector field. A vector field associates a vector with every point in a Euclidean space. Widely used in physics to model speed and direction of a moving object or strength and direction of a Newtonian force.

Here is my program:

public class Vector {
    private final double[] coordinates;

    public Vector(double[] coordinates) {
        this.coordinates = coordinates;
    }

    private int getCoordinatesLength() {
        return coordinates.length;
    }

    public double getCoordinate(int index) {
        return coordinates[index - 1];
    }

    public double getLength() {
        double sumOfCoordinatesSquared = 0;
        for (int i = 0; i < getCoordinatesLength(); i++) {
            sumOfCoordinatesSquared += getCoordinate(i + 1) * getCoordinate(i + 1);
        }
        return Math.sqrt(sumOfCoordinatesSquared);
    }

    private double getDirection2D() {
        return Math.atan(getCoordinate(2) / getCoordinate(1));
    }

    public double[] getDirection() {
        if (getCoordinatesLength() != 2 && getCoordinatesLength() != 3) {
            throw new IllegalArgumentException("dimention of the vector must be either 2 or 3");
        }
        int dimention = 0;
        if (getCoordinatesLength() == 2) dimention = 1;
        else if (getCoordinatesLength() == 3) dimention = 2;
        double[] angles = new double[dimention];
        if (getCoordinatesLength() == 2) {
            angles[0] = Math.atan(getCoordinate(2) / getCoordinate(1));
        } else if (getCoordinatesLength() == 3) {
            double vectorLength = getLength();
            double azimuth = Math.atan(getCoordinate(2) / getCoordinate(1));
            double zenith = Math.acos(getCoordinate(3) / vectorLength);
            angles[0] = azimuth;
            angles[1] = zenith;
        }
        return angles;
    }

    public Vector add(Vector otherVector) {
        if (getCoordinatesLength() != otherVector.getCoordinatesLength()) {
            throw new IllegalArgumentException("length of the vectors must be equal");
        }
        double[] newCoordinates = new double[getCoordinatesLength()];
        for (int i = 0; i < getCoordinatesLength(); i++) {
            newCoordinates[i] = getCoordinate(i + 1) + otherVector.getCoordinate(i + 1);
        }
        return new Vector(newCoordinates);
    }

    public Vector multiplyByScalar(double scalar) {
        double[] newCoordinates = new double[getCoordinatesLength()];
        for (int i = 0; i < getCoordinatesLength(); i++) {
            newCoordinates[i] = getCoordinate(i + 1) * scalar;
        }
        return new Vector(newCoordinates);
    }

    public Vector subtract(Vector otherVector) {
        return add(otherVector.multiplyByScalar(-1.0));
    }

    public boolean isEqual(Vector otherVector) {
        if (getCoordinatesLength() != otherVector.getCoordinatesLength()) return false;
        for (int i = 0; i < getCoordinatesLength(); i++) {
            if (getCoordinate(i + 1) != otherVector.getCoordinate(i + 1)) return false;
        }
        return true;
    }

    public double applyDotProduct(Vector otherVector) {
        if (getCoordinatesLength() != otherVector.getCoordinatesLength()) {
            throw new IllegalArgumentException("length of the vectors must be equal");
        }
        double dotProduct = 0;
        for (int i = 0; i < getCoordinatesLength(); i++) {
            dotProduct += getCoordinate(i + 1) * otherVector.getCoordinate(i + 1);
        }
        return dotProduct;
    }

    public Vector applyCrossProduct(Vector otherVector) {
        if (getCoordinatesLength() != otherVector.getCoordinatesLength()) {
            throw new IllegalArgumentException("length of the vectors must be equal");
        }
        if (getCoordinatesLength() != 3) {
            throw new IllegalArgumentException("dimention of the vector must be 3");
        }
        int x = 1;
        int y = 2;
        int z = 3;
        double newXCoordinate = getCoordinate(y) * otherVector.getCoordinate(z) - getCoordinate(z) * otherVector.getCoordinate(y);
        double newYCoordinate = getCoordinate(z) * otherVector.getCoordinate(x) - getCoordinate(x) * otherVector.getCoordinate(z);
        double newZCoordinate = getCoordinate(x) * otherVector.getCoordinate(y) - getCoordinate(y) * otherVector.getCoordinate(x);
        double[] newCoordinates = {
            newXCoordinate,
            newYCoordinate,
            newZCoordinate
        };
        return new Vector(newCoordinates);
    }

    public boolean isPerpendicular(Vector otherVector) {
        if (applyDotProduct(otherVector) == 0) return true;
        else return false;
    }

    public boolean isParallel(Vector otherVector) {
        double scalingFactor = 0;
        for (int i = 0; i < getCoordinatesLength(); i++) {
            if (getCoordinate(i + 1) != 0 && otherVector.getCoordinate(i + 1) != 0) {
                scalingFactor = getCoordinate(i + 1) / otherVector.getCoordinate(i + 1);
                break;
            }
        }
        double[] newCoordinates = new double[getCoordinatesLength()];
        for (int i = 0; i < getCoordinatesLength(); i++) {
            newCoordinates[i] = getCoordinate(i + 1) / scalingFactor;
        }
        Vector newVector = new Vector(newCoordinates);
        if (otherVector.isEqual(newVector)) return true;
        else return false;
    }

    public String toString() {
        String printedCoordinates = "";
        for (int i = 0; i < getCoordinatesLength() - 1; i++) {
            printedCoordinates += (getCoordinate(i + 1) + ", ");
        }
        return "[" + printedCoordinates + getCoordinate(getCoordinatesLength()) + "]";
    }

    public void draw(double originX, double originY, double scaleDownFactor, double arrowHeadSize) {
        if (getCoordinatesLength() != 2) {
            throw new IllegalArgumentException("dimention of the vector must be 3");
        }
        double newX = getCoordinate(1) * scaleDownFactor;
        double newY = getCoordinate(2) * scaleDownFactor;
        double arrowHeadPointX = originX + newX;
        double arrowHeadPointY = originY + newY;
        StdDraw.line(originX, originY, arrowHeadPointX, arrowHeadPointY);
        double arrowHeadBaseX = arrowHeadSize * Math.sin(getDirection2D());
        double arrowHeadBaseY = arrowHeadSize * Math.cos(getDirection2D());
        double[] arrowHeadXCoordinates = {-arrowHeadBaseX + (originX + 0.95 * newX),
                                           arrowHeadBaseX + (originX + 0.95 * newX),
                                           arrowHeadPointX
        };
        double[] arrowHeadYCoordinates = {
             arrowHeadBaseY + (originY + 0.95 * newY),
            -arrowHeadBaseY + (originY + 0.95 * newY),
             arrowHeadPointY
        };
        StdDraw.filledPolygon(arrowHeadXCoordinates, arrowHeadYCoordinates);
    }

    public static void main(String[] args) {
        /*
        double[] coordinatesOfVectorA = {1,2};
        double[] coordinatesOfVectorB = {0,1};
        Vector vectorA = new Vector(coordinatesOfVectorA);
        Vector vectorB = new Vector(coordinatesOfVectorB);
        double originX = 0.5;
        double originY = 0.5;
        double scaleDownFactor = 0.1; 
        double arrowHeadSize = 0.01;

        System.out.println("Vector A = " + vectorA.toString());
        System.out.println("Vector B = " + vectorB.toString());
        System.out.println("A plus B equals " + vectorA.add(vectorB).toString());
        System.out.println("A minus B equals " + vectorA.subtract(vectorB).toString());
        System.out.println("Dot product of A and B equals " + vectorA.applyDotProduct(vectorB));
        //System.out.println("Cross product of A and B equals " + vectorA.applyCrossProduct(vectorB).toString()); 
        System.out.println(vectorA.isParallel(vectorB));

        vectorA.draw(originX, originY, scaleDownFactor, arrowHeadSize);
        vectorB.draw(originX, originY, scaleDownFactor, arrowHeadSize);
        */
        StdDraw.setXscale(-1, 1);
        StdDraw.setYscale(-1, 1);
        for (int i = -10; i < 11; i++) {
            for (int j = -10; j < 11; j++) {
                if (i == 0 && j == 0) j++;
                double x = 1.0 * i / 10;
                double y = 1.0 * j / 10;
                double vectorXCoordinate = -y;
                double vectorYCoordinate = x;
                double[] coordinates = {
                    vectorXCoordinate,
                    vectorYCoordinate
                };
                Vector vector = new Vector(coordinates);
                vector.draw(x, y, 0.1, 0.01);
            }
        }
    }
}

StdDraw is a simple API written by the authors of the book. I checked my program and it works. Here is one instance of it:

Input (taken from here):

enter image description here

Output:

enter image description here

Is there any way that I can improve my program?

Thanks for your attention.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Comment from another user who lacks ability to comment currently: "The question is very broad and consequently there are many improvements you can make. You will likely get more meaningful answers if you state explicitly what do you want to improve. Some examples are "I want it to run faster", "I want it to be able to manage large datasets", "I want to present the solution to students", etc." \$\endgroup\$ Sep 28 '20 at 21:12
4
\$\begingroup\$

I have some suggestions for your code.

Extract the expression to variables when used multiple times.

You have multiple instances where you use the method getCoordinatesLength / getCoordinate multiple times in the same method. In your code, you can extract the similar expressions into variables; this will make the code shorter and easier to read.

Simplify the boolean conditions.

Generally, when you are returning both true and false surrounded by a condition, you know you can refactor the logic of the expression.

Before

public boolean isPerpendicular(Vector otherVector) {
   if (applyDotProduct(otherVector) == 0) return true;
   else return false;
}

After

public boolean isPerpendicular(Vector otherVector) {
   return applyDotProduct(otherVector) == 0;
}

Before

public boolean isParallel(Vector otherVector) {
    //[...]
   if (otherVector.isEqual(newVector)) return true;
   else return false;
}

After

public boolean isParallel(Vector otherVector) {
    //[...]
   return otherVector.isEqual(newVector);
}

Use java.lang.StringBuilder to concatenate String in a loop.

It's generally more efficient to use the builder in a loop, since the compiler is unable to optimize it by itself while translating your code into bytecode; The compiler will not use the java.lang.StringBuilder in complex loops and your method will take more time and more memory to execute, since the String Object is immutable (a new instance will be created each iteration).

Before

public String toString() {
   String printedCoordinates = "";
   for (int i = 0; i < getCoordinatesLength() - 1; i++) {
      printedCoordinates += (getCoordinate(i + 1) + ", ");
   }
   return "[" + printedCoordinates + getCoordinate(getCoordinatesLength()) + "]";
}

After

public String toString() {
   StringBuilder printedCoordinates = new StringBuilder();
   for (int i = 0; i < getCoordinatesLength() - 1; i++) {
      printedCoordinates.append(getCoordinate(i + 1)).append(", ");
   }
   return "[" + printedCoordinates + getCoordinate(getCoordinatesLength()) + "]";
}

Vector#getDirection method

This method can be shortened by merging the conditions, using anonymous arrays and in-lining the variables.

Before

public double[] getDirection() {
   //[...]
   int dimention = 0;
   if (getCoordinatesLength() == 2) dimention = 1;
   else if (getCoordinatesLength() == 3) dimention = 2;
   double[] angles = new double[dimention];
   if (getCoordinatesLength() == 2) {
      angles[0] = Math.atan(getCoordinate(2) / getCoordinate(1));
   } else if (getCoordinatesLength() == 3) {
      double vectorLength = getLength();
      double azimuth = Math.atan(getCoordinate(2) / getCoordinate(1));
      double zenith = Math.acos(getCoordinate(3) / vectorLength);
      angles[0] = azimuth;
      angles[1] = zenith;
   }
   return angles;
}

After

public double[] getDirection() {
   int coordinatesLength = getCoordinatesLength();
   //[...]
   if (coordinatesLength == 2) {
      return new double[] {Math.atan(getCoordinate(2) / getCoordinate(1))};
   } else if (coordinatesLength == 3) {
      double atan = Math.atan(getCoordinate(2) / getCoordinate(1));
      double acos = Math.acos(getCoordinate(3) / getLength());
      return new double[] {atan, acos};
   } else {
      return new double[0]; // You can also throw an exception, null, etc.
   }
}
\$\endgroup\$
3
\$\begingroup\$

Knowing that Vector is already a common class in Java, choosing Vector as a name for another class becomes quite confusing. Since this is specifically an Euclidean vector, you should literally name the class as such: EuclideanVector.

The class is intended to be immutable but it's constructor exposes the internal data structure to external components and allows them to alter the object state after it has been initialized (this is a bug). The input array to constructor should not be stored as such. It should be cloned:

public Vector(double[] coordinates) {
    this.coordinates = (double[]) coordinates.clone();
}

The getCoordinatesLength() suggests that the internal implementation is an array or a list but the getCoordinate(int) method requires a 1-based index, instead of being 0-based that is prevalent in everywhere else in Java. The getCoordinatesLength() should be renamed to getComponentCount() and the indexing should be changed to start from 0. That way you save youself from all the "+1,-1"-juggling inside the class.

Likewise the getCoordinate(int) method should be renamed getComponent(int) as that is the correct mathematical term.

The getDirection2D() method assumes that vector has at least two components but there is no validation. The user gets an ugly ArrayIndexOutOfBounds error without clarification. Add a check that there are enough components and throw an exception with specific information.

The Vector class is riddled with the same three magic numbers all over again. Replace numeric constants 1, 2 and 3 with constant fields X, Y and Z and document them so that the user knows the values can be passed to the getComponent(int) method.

The draw(double, double, double, double) absolutely does not belong in a vector class. Drawing the vector belongs in a UI component, not in a data structure. This violates the single responsibility principle. You should add a dedicated class for drawing an EuclideanVectorField. That might as well be a separate code review.

I have a feeling that pretty much all of the public methods should also be final. This way your implementation of methods that rely on other methods can not be altered by subclassing and overriding the other methods.

\$\endgroup\$
5
  • \$\begingroup\$ So draw should be a static method inside a test client? If yes, then why in a turtle graphics class we use draw to make computations simpler? And also could you please explain the last sentence more? Possibly with a before-after example. I appreciate your help. \$\endgroup\$ Sep 29 '20 at 9:44
  • \$\begingroup\$ I cannot review your turtle graphics example as I don't have code for it, but let me say that just because it was given to you by a professor does not mean it's good code. :) \$\endgroup\$ Sep 29 '20 at 9:47
  • \$\begingroup\$ I am self-learning from the above mentioned book. Here is the turtle graphics example codereview.stackexchange.com/questions/249564/… \$\endgroup\$ Sep 29 '20 at 9:49
  • 1
    \$\begingroup\$ You should check SOLID-principles. It is the single most important thing (or 5 things actually) I have ever learned about software development. en.wikipedia.org/wiki/SOLID . You should keep in mind that the code you are given is classroom code which takes shortcuts to make learning easier compared to what you're expected to do once you have graduated. \$\endgroup\$ Sep 29 '20 at 9:54
  • \$\begingroup\$ Oh...! Thank you very much for this clarification. :) \$\endgroup\$ Sep 29 '20 at 9:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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