10
\$\begingroup\$

This basically it sets a shapes default size and radius based on how many characters are in a string that is passed to it, the user gets the option to manually set the Font, otherwise it is automatically set for them. All letters are set to face towards the center of the circle as it wraps around. I do seem to have two minor flaws in the algorithm, the circular text is slightly offset from the circle object to the right I think and so I have to manually adjust the padding around the sides based on the size of the circle generated.

Another flaw is that as more text is placed in the circle the beginning and end doesn't have consistent spacing with the rest of the sentence, it has a bit of an extra gap which seems to get slightly bigger as more characters are placed in the circle.

If you look at the picture on the right, just noticed this, some of the letters are slightly offset just by a few pixels:

Here is a test run one was set manually (left), the other using default settings (right)

Now I was only required to manually enter the positions of each letter, but I decided to go above and beyond so I would have reusable code for future projects.

/** Main */
import javafx.application.Application;
import javafx.geometry.Insets;
import javafx.geometry.Pos;
import javafx.scene.Scene;
import javafx.scene.layout.GridPane;
import javafx.stage.Stage;

/**
 * Created by John on 7/11/2014.
 */
public class Prog14_05 extends Application {
    @Override
    public void start(Stage primaryStage) {
        // Create Pane
        circularText phrase = new circularText("WE ARE ANONYMOUS ",
                480, 480);
        phrase.setFont("Matrix");
        phrase.setTextSize(30);

        // Place clock and label in border pane
        GridPane pane = new GridPane();
        pane.setAlignment(Pos.CENTER);
        pane.setPadding(new Insets(15, 30, 30, 0));
        pane.setStyle("-fx-background-color: black");
        pane.getChildren().add(phrase);

        // Create a scene and place it in the stage
        Scene scene = new Scene(pane);
        primaryStage.setTitle("Exercise14_05");
        primaryStage.setScene(scene);
        primaryStage.show();
    }
}

/** circularText class */
import javafx.scene.layout.Pane;
import javafx.scene.paint.Color;
import javafx.scene.shape.Circle;
import javafx.scene.text.Font;
import javafx.scene.text.Text;

/**
 * Created by John on 7/11/2014.
 */
public class circularText extends Pane {
    double textSize = 30;
    String string = "";
    String fontName = "";
    Font font = new Font("Times Roman", textSize);
    // Pane's width and height
    private double w = 250, h = 250;

    /** Create Constructor */
    public circularText (String phrase, double w, double h) {
        this.w = w;
        this.h = h;
        this.string = phrase;
        textSize = (this.w / this.string.length()) * 2;
        paintText(this.string, this.font);
    }

    /** Set new font */
    public void setFont(String name) {
        Font font = new Font(name, textSize);
        this.font = font;
        this.fontName = name;
        paintText(this.string, this.font);
    }

    /** Return textSize */
    public double getTextSize() {
        return this.textSize;
    }

    /** Set textSize */
    public void setTextSize(double textSize) {
        this.textSize = textSize;
        Font font = new Font(fontName, textSize);
        this.font = font;
        paintText(this.string, this.font);
    }

    /** Return pane's width */
    public double getW() {
        return w;
    }

    /** Set pane's width */
    public void setW(double w) {
        this.w = w;
        textSize = (this.w / this.string.length()) * 2;
        paintText(this.string, this.font);
    }

    /** Return pane's height */
    public double getH() {
        return h;
    }

    /** Set pane's height */
    public void setH(double h) {
        this.h = h;
        textSize = (this.w / this.string.length()) * 2;
        paintText(this.string, this.font);
    }

    /** Paint the Letters */
    protected void paintText(String phrase, Font font) {
        // Initialize parameters
        double clockRadius = Math.min(w, h) * 0.8 * 0.5;
        double centerX = w / 2;
        double centerY = h / 2;

        // Draw circle
        Circle circle = new Circle(centerX, centerY, clockRadius);
        circle.setFill(null);
        circle.setStroke(null);
        getChildren().clear();
        getChildren().add(circle);

        // Place text in a circular pattern
        int i = 0;
        double degree = 360 / phrase.length();
        for (double degrees = 0; i < phrase.length(); i++, degrees += degree) {
            double pointX = circle.getCenterX() + circle.getRadius() *
                Math.cos(Math.toRadians(degrees));
            double pointY = circle.getCenterY() + circle.getRadius() *
                Math.sin(Math.toRadians(degrees));
            Text letter = new Text(pointX, pointY, phrase.charAt(i) + "");
            letter.setFont(font);
            letter.setFill(Color.LIME);
            letter.setRotate(degrees + 90);
            getChildren().add(letter);
        }

    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review. Be aware that modifying what your code actually does is not on-topic for this site. While we can review the code you have, correcting the issues that you have with your code is not what this site is about. Code Review is about making your code do the same thing in a cleaner way! \$\endgroup\$ Jul 13 '14 at 22:06
  • \$\begingroup\$ I understand, I'll be sure to keep that in mind next time \$\endgroup\$ Jul 13 '14 at 22:51
7
\$\begingroup\$

Naming

Your class is named circularText, it should be named CircularText as all classes in Java should start with a capital character.


Declared fields

You have declared two fields as private but what about the rest?

double textSize = 30;
String string = "";
String fontName = "";
Font font = new Font("Times Roman", textSize);

All these should be marked private. In addition, string should be marked final and does not need to be initialized there as it gets a new value in the constructor.

Your textSize gets a value in the constructor so the value 30 is only used for initializing the font. This may lead to potential bugs. Your font variable should also be initialized in the constructor, after the textSize value.

Your String string; has a horrible name. I can see that it is a string, what is it used for? A better name would be text or phrase.


Width and height fields

// Pane's width and height
private double w = 250, h = 250;
  • Please only declare one field per line, it gets easier to read that way.
  • Instead of adding a comment // Pane's width and height, why not name the variables as width and height directly? Then you don't need that comment. Comments should describe why, not the what.
  • These variables also get a value in the constructor so initializing them to 250 here is useless.
  • The getters and setters for these are just as badly named as the variables themselves, getWidth/setHeight/etc. would be a lot better names for those methods.

Circle, what circle?

Circle circle = new Circle(centerX, centerY, clockRadius);
circle.setFill(null);
circle.setStroke(null);
getChildren().add(circle);

I'm not sure what purpose your Circle fills, I tried removing this code and I did not see any change. You also don't provide any way to provide any paint to the circle, and when I tried adding some color to the circle it's probably a good thing that you didn't.

Circle with color


Char to String

phrase.charAt(i) + "" is a dirty way of converting a char to a string. A better method would be String.valueOf(phrase.charAt(i)).


Double precision

double degree = 360 / phrase.length();

Even though you store the result as a double, you're actually using int division here which makes the result an int. To add some accuracy, you have to specify one of the ints as being a double, a simple way to do this is:

double degree = 360.0 / phrase.length();

Missing flexibility

You provide some ways of altering the text, but you do not provide a way to change this:

letter.setFill(Color.LIME);

Or this:

letter.setRotate(degrees + 90);

Summary

It is a nice component you are building here that I see a lot of use-cases for, but after you've fixed the bugs with the positioning, please provide some more flexibility for users of your component to modify more aspects of it. I like a lot that you are using JavaFX.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thanks I appreciate your review, this helps a lot...I did notice the lack of privacy in my datafields after posting but a lot of the other things you mentioned slipped my mind. Thanks for the critique and I'll be sure to make these changes and add more flexibility. :) \$\endgroup\$ Jul 16 '14 at 16:37
  • \$\begingroup\$ By the way, I did fail to remove the circle. It was primarily there for trouble shooting my algorithm. I suppose once I finish working out those bugs it will have served it's purpose \$\endgroup\$ Jul 16 '14 at 16:39
  • \$\begingroup\$ I got it @Simon André Forsberg, everything seems to work correctly now and I made all the fixes you suggested. The problem with my algorithm was the circle object that I incorporated into my algorithm for some reason. \$\endgroup\$ Jul 16 '14 at 18:37

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.