How many squares can you see?

Question

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 < points * sideC; i += sideC) {
            for (int j = 0; j < points * sideC; j += sideC) {
                g2D.drawRect(offSet + i, offSet + j, sideC, sideC);
            }
        }
        // Paint the red square.
        if (sideC * points >= 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 <= points * sideC && !stop) {
            while (yRed < offSet + points * sideC && !stop) {
                long s = System.currentTimeMillis();
                repaint();
                xRed += sideC;
                if (xRed >= offSet + (points + 1) * sideC - side) {
                    xRed = offSet;
                    yRed += sideC;
                }
                if (yRed >= offSet + (points + 1) * sideC - side) {
                    yRed = offSet;
                    xRed = offSet - sideC;
                    break;
                }
                System.out.println(xRed + "\t" + yRed + "\t" + side);
                try {
                    s = System.currentTimeMillis()- s;
                    s = s > 0 ? s : 500;
                    t.sleep(updatePeriod - s);
                } catch (InterruptedException ie) {}
            }
            side += sideC;
        }
    }

    public void start() {
        System.out.println("Starting");
        stop = false;
        t = new Thread(this, "Hello!");
        t.start();
    }

    public void stop() {
        System.out.println("Stopping");
        stop = true;
        t = null;
    }
}

The class extending JFrame is a very simple one (till now):

import javax.swing.JFrame;
import javax.swing.SwingUtilities;

@SuppressWarnings("serial")
public class Squares extends JFrame {

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

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {
        SwingUtilities.invokeLater(() -> { // Java 8 - lambda expression
            new Squares();
        });
    }
}

---

Sample output is:

---

Any help regarding simplification of the code, refactoring, cleaning up, improvisation, improving efficiency, the overall technique through comments, answers, downvotes (and perhaps upvotes?) would be greatly appreciated. Remarks on improving its elegance would be great!

Answer 1

1. 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);
       }
       ...
   }
   

2. 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;
   }
   

3. 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.

4. 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.)

5. 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

6. 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);
   

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

8. 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.

9. 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;
   

10. 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);
    

11. Use method names instead of commenting:

    // paint the main grid.
    for (int i = 0; i < points * sideC; i += sideC) {
        for (int j = 0; j < points * sideC; j += sideC) {
            g2D.drawRect(offSet + i, offSet + j, sideC, sideC);
        }
    }
    // Paint the red square.
    if (sideC * points >= 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 < points * sideC; i += sideC) {
            for (int j = 0; j < points * sideC; j += sideC) {
                g2D.drawRect(offSet + i, offSet + j, sideC, sideC);
            }
        }
    }
    
    private void paintRedSquare(Graphics2D g2D) {
        if (sideC * points >= 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 paintMainGrid and paintRedSquare if you need that but usually readers/maintainers happier with a higher level overview of the method which is easier to understand. (This answer and question contain another example.)

    (Clean Code by Robert C. Martin, Don’t Use a Comment When You Can Use a Function or a Variable, p67)

Answer 2

A few minor notes about the nested loops:

1. First, I'd create guard clauses for the stop flag instead of the &&. I've found that easier to follow:

   while (side <= points * sideC) {
       if (stop) {
           return;
       }
       while (yRed < offSet + points * sideC) {
           if (stop) {
               return;
           }
           ...
       }
       ...
   
   }
   

   (If you use Swing Timers, I guess you won't need it at all.)

2. Then, points is final, offset and sideC are static final, so you could use a few explanatory variable before the loops:

   final int maxSide = points * sideC;
   while (side <= maxSide) {
       if (stop) {
           return;
       }
       final int maxYRed = offSet + maxSide;
       while (yRed < maxYRed) {
           if (stop) {
               return;
           }
   

   Feel free to choose better names, I'm just guessing here.

   (Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

3. offSet + (points + 1) * sideC also never changes it could also have an explanatory variable.

That's all I have now, maybe somebody could do a more in depth review. If not, feel free to post a follow-up question with the refactored code.