HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Controlling a robot arm in a Java GUI program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
robotprogramjavacontrollingarmgui

Problem

I have this funny program for playing around with 2D vectors:

Vector2D.java:

```
package net.coderodde.robohand;

import java.awt.geom.Point2D;
import java.util.List;
import java.util.ArrayList;
import java.util.Objects;

/**
* This class implements a vector. The tail of another vector can be attached to
* the head of this vector.
*
* @author Rodion "rodde" Efremov
* @version 1.6 (Apr 17, 2016)
*/
public class Vector2D {

/**
* The x-coordinate of the tail of this vector.
*/
private float locationX;

/**
* The y-coordinate of the tail of this vector.
*/
private float locationY;

/**
* The length of this vector.
*/
private float length;

/**
* The angle of this vector in radians.
*/
private float angle;

/**
* The list of all the child vectors of this vector. Child vectors' tails
* are attached to the head of this vector.
*/
private final List childVectors = new ArrayList<>();

/**
* Constructs a vector with given length and angle.
*
* @param length the length of a new vector.
* @param angle the angle in radians of a new vector.
*/
public Vector2D(float length, float angle) {
this.length = length;
this.angle = angle;
}

/**
* Connects {@code vector} as a child to this vector.
*
* @param vector the child vector to add.
*/
public void addChildVector(Vector2D vector) {
Objects.requireNonNull(vector, "The input vector is null.");
childVectors.add(vector);
Point2D.Float headLocation = getHeadLocation();
vector.setLocation(headLocation.x, headLocation.y);
}

/**
* Removes a child vector if it is connected to the tail of this vector.
*
* @param vector the vector to remove from the child list.
*/
public void removeChildVector(Vector2D vector) {
Objects.requireNonNull(vector, "The input vector is null.");
childV

Solution

float x = locationX + length * (float) Math.cos(angle);
    float y = locationY + length * (float) Math.sin(angle);
    point.x = x;
    point.y = y;


These local stores don't add any relevant information and should just be written directly to point's variables.

/**
 * Chooses a preceding vector in the list as selected. If the current 
 * selected vector is first in the list, does nothing.
 */
public void decSelectedVectorIndex() {
    if (selectedVectorIndex > 0) {
        selectedVectorIndex--;
    }

    repaint();
}



If the current selected vector is first in the list, does nothing.

So what does repaint() do? Probably not nothing.

/**
 * Makes selected vector a little bit longer.
 */
public void enlargeCurrentSelectedVector() {
    setSelectedVectorLength(LENGTH_DELTA);
}



longer

"set" implies "becomes equal to". But here it adds, instead.

public void rotateSelectedVector(float angleDelta) {
    float newAngle = vectorList.get(selectedVectorIndex)
                               .getAngle() + angleDelta;
    vectorList.get(selectedVectorIndex).setAngle(newAngle);
    repaint();
}


You have to break it up into multiple lines because you don't want to pull the vector out of the list. This makes for a lengthy line...

public void rotateSelectedVector(float angleDelta) {
    Vector2D selectedVector = vectorList.get(selectedVectorIndex);
    float newAngle = selectedVector.getAngle() + angleDelta;
    selectedVector.setAngle(newAngle);

    repaint();
}


But even when edited it looks weird. Your getters/setters are hindering you here, ideally you'd just do something like this:

selectedVector.angle += angleDelta;


Because we can't do this without making access public, the second alternative is making a method that will allow you to add a value.

/**
 * Gets the angle of this vector.
 * 
 * @return the angle of this vector.
 */
public float getAngle() {
    return angle;
}

/**
 * Sets the angle of this vector. If this vector has child vectors, it 
 * updates their location and angles as well.
 * 
 * @param angle the angle in radians.
 */
public void setAngle(float angle) {
    float angleDelta = angle - this.angle;
    this.angle = angle;
    Point2D.Float headLocation = getHeadLocation();

    for (Vector2D childVector : childVectors) {
        childVector.setLocation(headLocation.x, headLocation.y);
        childVector.setAngle(childVector.getAngle() + angleDelta);
    }
}


But it looks like your setter is actually quite complex, so we definitely cannot get rid of the setter here. I think a helper which takes angleDelta would be useful.

...

Heck, looking at it again, I think making the set a helper and the delta the real one is the better choice.

public void addAngle(float angleDelta) {
    this.angle += angleDelta;
    Point2D.Float headLocation = getHeadLocation();

    for (Vector2D childVector : childVectors) {
        childVector.setLocation(headLocation.x, headLocation.y);
        childVector.addAngle(angleDelta);
    }
}


You save having to calculate the delta for every child.

public void update(Graphics g) {
    vectorList.get(0).setLocation(getWidth() / 2, getHeight() / 2);


What's that for? On a repaint, you update your model. Why does looking at data change the data? Could use a comment. Or a wrapping function. (I get the feeling it's for centering the hand.)

Code Snippets

float x = locationX + length * (float) Math.cos(angle);
    float y = locationY + length * (float) Math.sin(angle);
    point.x = x;
    point.y = y;
/**
 * Chooses a preceding vector in the list as selected. If the current 
 * selected vector is first in the list, does nothing.
 */
public void decSelectedVectorIndex() {
    if (selectedVectorIndex > 0) {
        selectedVectorIndex--;
    }

    repaint();
}
/**
 * Makes selected vector a little bit longer.
 */
public void enlargeCurrentSelectedVector() {
    setSelectedVectorLength(LENGTH_DELTA);
}
public void rotateSelectedVector(float angleDelta) {
    float newAngle = vectorList.get(selectedVectorIndex)
                               .getAngle() + angleDelta;
    vectorList.get(selectedVectorIndex).setAngle(newAngle);
    repaint();
}
public void rotateSelectedVector(float angleDelta) {
    Vector2D selectedVector = vectorList.get(selectedVectorIndex);
    float newAngle = selectedVector.getAngle() + angleDelta;
    selectedVector.setAngle(newAngle);

    repaint();
}

Context

StackExchange Code Review Q#125948, answer score: 2

Revisions (0)

No revisions yet.