How many squares can you see?

With a positive outlook, I posted this challenge at Programming Puzzles & Code Golf. I tried to come up with my own solution to the challenge which is as follows:

The output is displayed (not yet exported to GIF) on a class extending JPanel:

```
import java.awt.BasicStroke;
import java.awt.Color;
import java.awt.Graphics;
import java.awt.Graphics2D;
import javax.swing.JPanel;

@SuppressWarnings("serial")
public class Display extends JPanel implements Runnable {
private int points;
private int xRed;
private int yRed;
private int side;
// Side of the smallest square:
private static final int sideC = 80;
private static final int offSet = 20;
private Thread t;
private boolean stop;
private static final long updatePeriod = 1000;

public Display(int points) {
this.side = sideC;
this.points = points;
yRed = offSet;
xRed = yRed - side;
stop = false;
repaint();
start();
}

@Override
public void paint(Graphics g) {
super.paint(g);
Graphics2D g2D = (Graphics2D) g;
// The stroke to apply.
BasicStroke stroke;
stroke = new BasicStroke(5.0f, BasicStroke.CAP_ROUND, BasicStroke.JOIN_ROUND);
g2D.setStroke(stroke);
// Let the painting begin!
g2D.setColor(Color.WHITE);
g2D.fillRect(0, 0, getWidth(), getWidth());

g2D.setColor(Color.BLACK);
// paint the main grid.
for (int i = 0; i = side) {
g2D.setColor(Color.RED);
g2D.drawRect(xRed, yRed, side, side);
} else {
side = sideC;
yRed = offSet;
xRed = offSet - sideC;
}
}

@Override
public void run() {
while (side = offSet + (points + 1) * sideC - side) {
xRed = offSet;
yRed += sideC;
}
if (yRed >= offSet + (points + 1) * sideC - side) {
yRed = off

-
A great comment from @tb-'s answer:


Do not extend JPanel, instead have a private JPanel
attribute and deal with it (Encapsulation).
There is no need to give access to all
JPanel methods for code dealing with a UserPanel
instance. If you extend, you are forced to stay with this forever,
if you encapsulate, you can change whenever you want without
taking care of something outside the class.

The is true for JFrame. It would also make @SuppressWarnings("serial") unnecessary.

So Squares would be something like this:

public class Squares {

    private final JFrame squareFrame = new JFrame();

    public Squares() {
        squareFrame.setSize(500, 500);
        squareFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        squareFrame.setContentPane(new Display(5));
        squareFrame.setVisible(true);
    }
    ...
}

-
According to Wikipedia:


all user interface components should be created and accessed only from the AWT event dispatch thread.

including repaint() in the run method (which is run by another thread). You should use SwingUtilities.invokeLater or invokeAndWait for this too:

try {
    SwingUtilities.invokeAndWait(() -> repaint());
} catch (InvocationTargetException e) {
    throw new RuntimeException(e);
} catch (InterruptedException e) {
    Thread.interrupted();
    break;
}

-
Be careful about thread-safety. If you call repaint() on EDT it will use xRed, yRed etc from multiple threads (EDT and your own thread), so you need proper synchronization for visibility:


[...] synchronization has no effect unless both read and write operations are synchronized.

Source: Effective Java, 2nd Edition, Item 66: Synchronize access to shared mutable data


Locking is not just about mutual exclusion; it is also about memory visibility.
To ensure that all threads see the most up-to-date values of shared mutable
variables, the reading and writing threads must synchronize on a common lock.

Source: Java Concurrency in Practice, 3.1.3. Locking and Visibility.

stop also suffers from proper synchronization. Changing its value (from another thread) might not be visible to your thread. For this I'd suggest you using an AtomicBoolean (which is thread safe).

For doing proper synchronization between the timer thread and EDT you need to restructure the code but I guess it would be much easier with Swing Timers than your custom thread. It looks like that it has been designed for exactly these kind of problems.

-
I would create another class for the Runnable/run implementation to fulfill the Single responsibility principle. Currently it has too many responsibilities: draws the current state, update the current state, starts/stops the updating thread. It could be splitted to at least three classes. (You might not need this if you use Swing Timers - I'm not too familiar with them.)

-
Calling overridable methods (like stop) from constructors is not a good practice. Child classes could see partially initialized objects in the overridden method. The stop should be final here. See What's wrong with overridable method calls in constructors? and Effective Java 2nd Edition, Item 17: Design and document for inheritance, or else prohibit it

-
The code calls a static method here on an instance which is misleading:

t.sleep(updatePeriod - s);

It should be the following:

Thread.sleep(updatePeriod - s);

-
The code never calls stop. If it's not necessary remove it, otherwise use it.

-
I'd use longer variable names than these ones:

private Thread t;
long s = System.currentTimeMillis();

Choose variable names which describes their purpose. t could be updateThread and s could be currentTimeMillis ro frameStartTime.

-
According to the Java Code Conventions, constants should be UPPERCASED_WITH_UNDERSCORES:

private static final int sideC = 80;
private static final int offSet = 20;
private static final long updatePeriod = 1000;

-

BasicStroke stroke;
stroke = new BasicStroke(5.0f, BasicStroke.CAP_ROUND, BasicStroke.JOIN_ROUND);

This could be one line:

BasicStroke stroke = new BasicStroke(5.0f, BasicStroke.CAP_ROUND, BasicStroke.JOIN_ROUND);

-
Use method names instead of commenting:

// paint the main grid.
for (int i = 0; i = side) {
    g2D.setColor(Color.RED);
    g2D.drawRect(xRed, yRed, side, side);
} else {
    side = sideC;
    yRed = offSet;
    xRed = offSet - sideC;
}

It could be:

paintMainGrid(g2D);
paintRedSquare(g2D);

...

private void paintMainGrid(Graphics2D g2D) {
    for (int i = 0; i = side) {
        g2D.setColor(Color.RED);
        g2D.drawRect(xRed, yRed, side, side);
    } else {
        side = sideC;
        yRed = offSet;
        xRed = offSet - sideC;
    }
}

Using these methods hides unnecessary details, increases the abstraction level of the code and it's easier to read. You still can check the details inside `pa

StackExchange Code Review Q#45681, answer score: 8