4
\$\begingroup\$

The app when works fine, but people are saying that the code is not written with good code style and following OOP principles.

Can I improve on any parts of my code? Especially regarding:

  • Code comments
  • Variable naming
  • Separation of concerns
  • Single responsibility principle
  • etc.
import javafx.application.Application;
import javafx.stage.Stage;
import javafx.animation.KeyFrame;
import javafx.animation.Timeline;
import javafx.event.ActionEvent;
import javafx.event.EventHandler;
import javafx.scene.Scene;
import javafx.util.Duration;
import java.util.Calendar;
import java.util.GregorianCalendar;
import javafx.geometry.Pos;
import javafx.scene.control.Button;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.HBox;
import javafx.scene.layout.Pane;
import javafx.scene.paint.Color;
import javafx.scene.shape.Circle;
import javafx.scene.shape.Line;
import javafx.scene.text.Text;
import javafx.application.Platform;

public class AnalogClockv2 extends Application {
  @Override // Override the start method in the Application class
  public void start(Stage primaryStage) {
    ClockPane clock = new ClockPane(); // Create a clock

    HBox hBox = new HBox(5);
    Button btStop = new Button("Stop");
    Button btStart = new Button("Start");
    hBox.getChildren().addAll(btStop, btStart);
    hBox.setAlignment(Pos.CENTER);

    BorderPane pane = new BorderPane();
    pane.setCenter(clock);
    pane.setBottom(hBox);

    // Create a scene and place it in the stage
    Scene scene = new Scene(pane, 250, 300);
    primaryStage.setTitle("Java Analog Clock Reloaded"); // Set the stage title
    primaryStage.setScene(scene); // Place the scene in the stage
    primaryStage.show(); // Display the stage


    clock.widthProperty().addListener(ov -> {
      clock.setW(pane.getWidth());
    });

    clock.heightProperty().addListener(ov -> {
      clock.setH(pane.getHeight());
    });
  }

  /**
   * The main method is only needed for the IDE with limited
   * JavaFX support. Not needed for running from the command line.
   */
  public static void main(String[] args) {
    launch(args);
  }

  class ClockPane extends Pane {
    private int hour;
    private int minute;
    private int second;

    // Clock pane's width and height
    private double w = 250, h = 250;

    private int sleepTime = 50;
    /**
     * Construct a default clock with the current time
     */
    public ClockPane() {
      setCurrentTime();

    }

    /**
     * Construct a clock with specified hour, minute, and second
     */
    public ClockPane(int hour, int minute, int second) {
      this.hour = hour;
      this.minute = minute;
      this.second = second;
      paintClock();
    }

    /**
     * Return hour
     */
    public int getHour() {
      return hour;
    }

    /**
     * Set a new hour
     */
    public void setHour(int hour) {
      this.hour = hour;
      paintClock();
    }

    /**
     * Return minute
     */
    public int getMinute() {
      return minute;
    }

    /**
     * Set a new minute
     */
    public void setMinute(int minute) {
      this.minute = minute;
      paintClock();
    }

    /**
     * Return second
     */
    public int getSecond() {
      return second;
    }

    /**
     * Set a new second
     */
    public void setSecond(int second) {
      this.second = second;
      paintClock();
    }

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

    /**
     * Set clock pane's width
     */
    public void setW(double w) {
      this.w = w;
      paintClock();
    }

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

    /**
     * Set clock pane's height
     */
    public void setH(double h) {
      this.h = h;
      paintClock();
    }

    /* Set the current time for the clock */
    public void setCurrentTime() {
      // Construct a calendar for the current date and time
      Calendar calendar = new GregorianCalendar();

      // Set current hour, minute and second
      this.hour = calendar.get(Calendar.HOUR_OF_DAY);
      this.minute = calendar.get(Calendar.MINUTE);
      this.second = calendar.get(Calendar.SECOND);

      paintClock(); // Repaint the clock
    }

    /**
     * Paint the clock
     */
    private void paintClock() {
      // Initialize clock 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(Color.WHITE);
      circle.setStroke(Color.BLACK);
      Text t1 = new Text(centerX - 5, centerY - clockRadius + 12, "12");
      Text t2 = new Text(centerX - clockRadius + 3, centerY + 5, "9");
      Text t3 = new Text(centerX + clockRadius - 10, centerY + 3, "3");
      Text t4 = new Text(centerX - 3, centerY + clockRadius - 3, "6");

      // Draw second hand
      double sLength = clockRadius * 0.8;
      double secondX = centerX + sLength
              * Math.sin(second * (2 * Math.PI / 60));
      double secondY = centerY - sLength
              * Math.cos(second * (2 * Math.PI / 60));
      Line sLine = new Line(centerX, centerY, secondX, secondY);
      sLine.setStroke(Color.RED);

      // Draw minute hand
      double mLength = clockRadius * 0.65;
      double xMinute = centerX + mLength
              * Math.sin(minute * (2 * Math.PI / 60));
      double minuteY = centerY - mLength
              * Math.cos(minute * (2 * Math.PI / 60));
      Line mLine = new Line(centerX, centerY, xMinute, minuteY);
      mLine.setStroke(Color.BLUE);

      // Draw hour hand
      double hLength = clockRadius * 0.5;
      double hourX = centerX + hLength
              * Math.sin((hour % 12 + minute / 60.0) * (2 * Math.PI / 12));
      double hourY = centerY - hLength
              * Math.cos((hour % 12 + minute / 60.0) * (2 * Math.PI / 12));
      Line hLine = new Line(centerX, centerY, hourX, hourY);
      hLine.setStroke(Color.GREEN);

      getChildren().clear();
      getChildren().addAll(circle, t1, t2, t3, t4, sLine, mLine, hLine);
    }
  }
}

For more context, the code is used to teach the fundamentals of OOP, how to use Java, and JavaFX in a high school CS course.

Please ask for clarification if there are issues running the code if needed, this is my first time on this site. Thanks!

\$\endgroup\$
3
\$\begingroup\$

Whoever says your code is not following OOP principles should continue by explaining their opinion about the shortcomings. As I see, it's class room code and it's intention is to teach a certain thing (JavaFX I assume) so it is acceptable to skip certain OOP aspects if it makes teaching the subject matter easier. When you want to teach OOP concepts, don't mix the JavaFX in it. Doing UI code with pure OOP is quite tedious and it will consume a lot of time from the actual teaching.

The code style needs some minor improvements. Technically, because the ClockPane is not public, it doesn't matter how it's properties are accessed as the class will not be extended, but for clarity it would be better if the property values were all set in a consistent manner, e.g. using the setter methods instead of direct access in setCurrentTime().

This change highlights the second problem: the scattering of paintClock() calls all over the class. Since ClockPane is not public, it makes very little sense to try to ensure the state is valid after each call since your own private code is the only thing that calls it. However, it the ClockPane was public, it would be better to provide a public user interface update method and document the update policy instead of forcing the clock to be painted three times if the caller tries to set the time using the setter methods.

Your coments look like they are generated by a bot and they are completely redundant. :) A setter method is well known to set a value given in the name so there is no need to repeat that in the comment unless the method does something else (which it does). If you added the important part to the comments it might highlight the problem with the paint calls:

/**
 * Set new hour and paint the clock.
 */

/**
 * Set new minute and paint the clock.
 */

/**
 * Set clock pane's height and paint the clock.
 */

...

Also, since you know about the single responsibility principle, the and words in the comments now should ring the alarm for the methods having two responsibilities.

There is no need to abbreviate width and height. So use getWidth() instead of getW().

Do not use end of line comments. They are very hard to maintain and the limited space forces them to be short which reduces their usefulness.

The comments should not desribe what the code does but why it is done. The code itself tells what it does in much greater detail than the comments can but the code cannot convey the reasoning for why it exists.

\$\endgroup\$
2
\$\begingroup\$

I will point out one thing as it relates to OOP. You can potentially run into issues when you extend Pane. If you are going to extend Pane, make the class final.

public final class ClockPane extends Pane

The other route is not to extend Pane. Using that route, you can create a method that returns the Clock as a Pane.

Clock clock = new Clock(..,..,..);
...
...
clock.getAsPane();

For more information about this potential pitfall, go here.

I created similar apps a while back. You can get some pointers from here and here.

The only other thing I would point out is the use of GregorianCalendar. It is an old API. It is recommended that LocalDate or LocalDateTime be used.

\$\endgroup\$

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.