Traffic simulation GUI: an exercise in concurrent programming using threads

Question

To avoid a code dump I have removed most of the code and left the first few lines of constructors and methods and anything relevant. Full code at the bottom.

The purpose of this code is to use threads to process the application. Each of the following components needs to run on separate threads: a current time stamp in 1-second intervals, a traffic light display for each intersection, and the speed and position of each "car" as it moves through the lights(obviously stopping at red).

Currently, the GUI accomplishes that, my question is more about whether the threads are accomplishing those objectives correctly. Can someone please review the code and give feedback on what I did right or ways to improve?

Light.java

public class Light {
    static Color[] colors = new Color[] { array of colors };
    int colorIndex;
    int seconds;
    Point pos;

    public int getSeconds() {return seconds;}
    public void setSeconds(int seconds) {}
    public Point getPos() {}
    public void setPos(Point pos) {}
    public void setColorIndex(int c) {}
    public Color getColor() {return colors[colorIndex];}
    public void transition() {}

Simulator.java

public class Simulator extends JFrame {
    ArrayList<Light> lights = new ArrayList<>();
    ArrayList<Car> cars = new ArrayList<>();
    int ticks = 0;
    Timer t;
    Random rand = new Random();
    JButton btnStart;
    JButton btnPause;

    Simulator() {
        btnStart = new JButton("Start");
        btnPause = new JButton("Pause");
        btnPause.setEnabled(false);
        t = new Timer(100, e -> {clockTicks();});
        btnStart.addActionListener(e -> {startSimulation();
                t.start();} else {t.stop();}});
        btnPause.addActionListener(e -> {t.stop();
                } else {t.start();}});}
    private void startSimulation() {
        lights.clear();
        cars.clear();
        for loop here}
        for loop here Car car = new Car();car.start();}}
    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {new Simulator();}});}
    private Light getLight(double x) {}}}
    private void clockTicks() {car.start();}
        for loop here}
    class Road extends JPanel {
    @Override protected void paintComponent(Graphics g) {
            super.paintComponent(g);
            for loop here}}for loop here}}}}}

Car.java

public class Car extends Thread {
    public static final Color[] colors = new Color[] { "colors" };
    private double x;
    private double velocity;
    private Color color;
    private Light light;

    public double getX() {}
    public void setX(double pos) {}
    public double getVelocity() {}}
    public void setVelocity(double velocity) {}
    public Color getColor() {}
    public void setColor(Color color) {}
    private Boolean isStopped() {}
    public void animate() {}}
    public Light getLight() {}
    public void setLight(Light light) {}
    @Override
    public void run() {
        while(x < 1200) {
            try {
                Thread.sleep(100);
                animate();
            } catch (InterruptedException e) {
                e.printStackTrace();}}}}

Complete Code

Light.java
package traffic;
import java.awt.Color;
import java.awt.Point;
public class Light {
    static Color[] colors = new Color[] { Color.red, Color.green, Color.yellow };
    int colorIndex;
    int seconds;
    Point pos;
    public int getSeconds() {
        return seconds;
    }
    public void setSeconds(int seconds) {
        this.seconds = seconds;
        if (this.seconds == 0) {
            transition();
        }
    }
    public Point getPos() {
        return pos;
    }
    public void setPos(Point pos) {
        this.pos = pos;
    }
    public void setColorIndex(int c) {
        this.colorIndex = c;

        if (colorIndex == 0) {
            // red get 15 seconds
            seconds = 15;
        }
        else if (colorIndex == 1) {
            seconds = 10;//green light
        } else {
            seconds = 3;//yellow light
        }
    }
    public Color getColor() {
        return colors[colorIndex];
    }
    public void transition() {
        // the light changes!
        colorIndex++;
        if (colorIndex == colors.length) {
            colorIndex = 0;//color index starts at 0
        }
        if (colorIndex == 0) {
            // red get 15 seconds
            seconds = 15;
        }
        else if (colorIndex == 1) {
            seconds = 10;//green light
        } else {
            seconds = 3;//yellow light
        }
    }
}



Simulator.java



package traffic;

import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.FlowLayout;
import java.awt.Graphics;
import java.awt.Point;
import java.util.ArrayList;
import java.util.Random;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.SwingUtilities;
import javax.swing.Timer;

@SuppressWarnings("serial")
public class Simulator extends JFrame {
    ArrayList<Light> lights = new ArrayList<>();
    ArrayList<Car> cars = new ArrayList<>();
    int ticks = 0;
    Timer t;
    Random rand = new Random();
    JButton btnStart;
    JButton btnPause;
    Simulator() {
        btnStart = new JButton("Start");
        btnPause = new JButton("Pause");
        btnPause.setEnabled(false);
        t = new Timer(100, e -> {
            clockTicks();
        });
        btnStart.addActionListener(e -> {
            if (btnStart.getText().equals("Start")) {
                startSimulation();
                t.start();
                btnStart.setText("Stop");
            } else {
                t.stop();
                btnStart.setText("Start");
                btnPause.setEnabled(false);
            }
        });
        btnPause.addActionListener(e -> {
            if (btnPause.getText().equals("Pause")) {
                btnPause.setText("Continue");
                t.stop();
            } else {
                t.start();
                btnPause.setText("Pause");
            }
        });
        JPanel jp = new JPanel();
        jp.setLayout(new BorderLayout());
        JPanel south = new JPanel();
        south.setLayout(new FlowLayout());
        south.add(btnStart);
        south.add(btnPause);
        jp.add(south, BorderLayout.SOUTH);
        Road road = new Road();
        jp.add(road, BorderLayout.CENTER);
        getContentPane().add(jp);
    }
    private void startSimulation() {
        lights.clear();
        cars.clear();
        btnPause.setEnabled(true);
        int n = rand.nextInt(2) + 3;
        int distance = 1000 / n;
        for (int i = 0; i < n; i++) {
            Point pos = new Point((i + 1) * distance, 350);
            Light l = new Light();
            l.setPos(pos);
            int c = rand.nextInt(3);
            l.setColorIndex(c);
            lights.add(l);
        }
        for (int i = 0; i < 3; i++) {
            Car car = new Car();
            int c = rand.nextInt(Car.colors.length);
            car.setColor(Car.colors[c]);
            int v = rand.nextInt(25);
            car.setVelocity(v);
            int x = rand.nextInt(500);
            car.setX(x);
            cars.add(car);
            car.start();
        }
    }
    public static void main(String[] args) {
        Simulator sim = new Simulator();
        sim.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        sim.setSize(1200, 700);
        sim.setVisible(true);
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                new Simulator();
            }
        });
    }
    private Light getLight(double x) {
        for (Light light : lights) {
            Point point = light.getPos();
            double diff = point.x - x;
            if (diff < 5 && diff > -1) {
                return light;
            }
        }
        return null;
    }
    private void clockTicks() {
        int n = rand.nextInt(100);
        ticks++;
        if (n == 5) {
            Car car = new Car();
            int c = rand.nextInt(Car.colors.length);
            car.setColor(Car.colors[c]);
            int velocity = rand.nextInt(25);
            car.setVelocity(velocity);
            car.setX(50);
            cars.add(car);
            car.start();
        }
        for (Car car : cars) {
            double x = car.getX();
            Light light = getLight(x);
            car.setLight(light);
        }
        if (ticks % 10 == 0) {
            for (Light light : lights) {
                int seconds = light.getSeconds();
                if (seconds > 0) {
                    light.setSeconds(seconds - 1);
                } else {
                    light.transition();
                }
            }
        }
        getContentPane().repaint();
    }
    class Road extends JPanel {
        @Override
        protected void paintComponent(Graphics g) {
            super.paintComponent(g);
            for (Light light : lights) {
                if (light != null) {
                    Point pos = light.getPos();
                    g.setColor(Color.BLACK);
                    g.drawString(Integer.toString(light.getSeconds()), pos.x, pos.y - 60);
                    Color c = light.getColor();
                    g.setColor(c);
                    g.fillOval(pos.x, pos.y, 40, 40);
                    g.drawLine(pos.x + 20, pos.y + 20, pos.x + 20, pos.y + 60);
                }
            }
            int i = 0;
            for (Car car : cars) {
                if (car != null) {
                    Color c = car.getColor();
                    g.setColor(c);
                    int x = (int) car.getX();
                    g.fillRect(x, 400, 20, 20);
                    g.setColor(Color.black);
                    g.drawString(String.format("(%.0f, 0) %.2f kmh", 
                            car.getX(), car.getVelocity()), x, 470 + (i % 5) * 10);
                    i++;
                }
            }
        }
    }
}



Car.java



package traffic;

import java.awt.Color;
public class Car extends Thread {
    public static final Color[] colors = new Color[] { Color.blue, Color.lightGray, Color.cyan, Color.magenta };
    private double x;
    private double velocity;
    private Color color;
    private Light light;
    public double getX() {
        return x;
    }
    public void setX(double pos) {
        this.x = pos;
    }
    public double getVelocity() {
        if(!isStopped()) {
            return velocity;
        }
        return 0;
    }
    public void setVelocity(double velocity) {
        this.velocity = velocity;
    }
    public Color getColor() {
        return color;
    }
    public void setColor(Color color) {
        this.color = color;
    }
    private Boolean isStopped() {
        if (light == null) return false;
        if(light.getColor().equals(Color.red)) return true;
        return false;
    }
    public void animate() {
        if (! isStopped()) {
            x += (velocity * 0.1);
        }
    }
    public Light getLight() {
        return light;
    }
    public void setLight(Light light) {
        this.light = light;
    }
    @Override
    public void run() {
        while(x < 1200) {
            try {
                Thread.sleep(100);
                animate();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

Answer 1

While the topic in question is Java Threads, I'd like to point out some general problems with the code of this solution, which will lead us to discovering some hidden (or not so much) bugs in it.

---

Leave blank lines inbetween parts of your code, it increases readability at a very low cost.

int seconds;
Name of the field isn't descriptive. Should be something like timeUntilSwitch.
Since you are providing getter & setter, the field itself should be private (so you can't access it directly from outside bypassing the getter). Same applies to pos.

int colorIndex;
There is no point in storing some arbitrary index, an enum value representing the color can be stored instead like so:

public enum LightColor {
    RED    ("GREEN",  15, Color.red),
    GREEN  ("YELLOW", 10, Color.green),
    YELLOW ("RED",    3,  Color.yellow);

    public final String next;
    public final int duration; // in seconds
    public final Color color;

    LightColor(String next, int duration, Color color) {
        this.next = next;
        this.duration = duration;
        this.color = color;
    }

    LightColor next() {
        return LightColor.valueOf(this.next);
    }
}

Back to your code:

if (colorIndex == 0) {
    // red get 15 seconds
    seconds = 15;
}

colorIndex isn't descriptive, so you had to use comments here to explain what's going on. This is a sign that refactoring is needed.

setColorIndex(int c) and transition() both do basically the same thing, they can be merged.

Car car = new Car();
int c = rand.nextInt(Car.colors.length);
car.setColor(Car.colors[c]);
int v = rand.nextInt(25);
car.setVelocity(v);
int x = rand.nextInt(500);
car.setX(x);
cars.add(car);
car.start();

Set-up code like this can be turned into a method of Car, startSimulation() is getting too long with all the details specified inside.

Simulator class doesn't abide the single-responsibility principle: split it into 2 classes, one of which does the application logic and the other handles the graphics.

if (btnPause.getText().equals("Pause")) {
    btnPause.setText("Continue");
    t.stop();
} else {
    t.start();
    btnPause.setText("Pause");
}

Relying on button names is bug-prone, if someone decides to change the name from "Pause" to "Paused", these conditions will break and you won't know why that happened. Create a state (e.g. bool paused) and follow it instead.

SwingUtilities.invokeLater(new Runnable() {
    public void run() {
        new Simulator();
    }
});

This entire block can be replaced with a lambda:
SwingUtilities.invokeLater(Simulator::new);
Although it does nothing, so you can just remove it instead.

if (diff < 5 && diff > -1) {
    return light;
}

If you increase maximal car velocity, cars will be able to ignore lights completely.

These are not the only problems; pay attention to magic numbers, disciptiveness of variable names, overly long methods and interdependence. For example in this piece of code...

if (ticks % 10 == 0) {
    for (Light light : lights) {
        int seconds = light.getSeconds();
        if (seconds > 0) {
            light.setSeconds(seconds - 1);
        } else {
            light.transition();
        }
    }
}

... we assume that one second equals 10 ticks. There is no explanation regarding this condition, you have to know the rest of the code to figure it out.

---

So let's get back to threads.

Lack of the clarity gives birth to errors: Car-threads keep running even when the timer is stopped, in fact there is no logic to prevent this. If you press Pause, wait a bit and press Continue, cars on the road will jump ahead, since they were moving the entire time, UI just wasn't being updated.

As for the other threads...

Each of the following components needs to run on separate threads: a current time stamp in 1-second intervals, a traffic light display for each intersection, and the speed and position of each "car" as it moves through the lights(obviously stopping at red).

... there are no other threads. None of the entities apart from cars use them.

---

There is some work to do, but I think it will be a good excercise. Good luck!

Answer 2

Your Light class, being a data model, should not care about colours, and only semantic states in an enum (i.e. stopping, stopping soon, or running). Prefer this over a colour or a colour index.

Do not track a Point in your light: the simulation model only cares about a horizontal offset, and the vertical offset is a rendering detail that should be pulled out to your GUI.

Get rid of your Light.setSeconds. Good encapsulation dictates that the light should know how to update its own time quantity.

This structure:

        if (colorIndex == 0) {
            // red get 15 seconds
            seconds = 15;
        }
        else if (colorIndex == 1) {
            seconds = 10;//green light
        } else {
            seconds = 3;//yellow light
        }

is better-represented with an expression-valued switch. Also notice that you've repeated that logic; don't do that.

Delete @SuppressWarnings("serial").

Your Simulator class needs to be split in two: a Simulator logic class with no GUI elements, and a SimulatorFrame class with only GUI elements.

Nearly all of your class members need to be made private final.

Move your button listeners from lambdas to class members since they're sufficiently large.

startSimulation exhibits a reuse antipattern. Rather than clearing and reinitializing lists in-place, you should be discarding and re-constructing a simulator object.

Don't rand.nextInt(2) + 3;; call the overload that has a lower and upper bound.

Add constant variables for magic numbers like 350.

Your invokeLater is not necessary.

I have not demonstrated this, but getLight needs to be refactored to use a binary search.

if (seconds > 0) is better written as if (seconds >= 1), since that's what you actually care about: being able to decrement without going negative. In the former implementation, if you switch to floating-point times you will get some nasty surprises.

light will never be null; I'm not sure why you check for that.

Don't have isStopped return a boxed Boolean; instead return boolean.

Car being its own thread is not a good idea; it should tick as a call from the main simulation timer. Otherwise, your pause is broken and only hides rendering but does not pause the car movement internally.

Your rendering uses too many magic numbers, and fails to prevent text overlap. Use font metrics to calculate and avoid these overlaps. The method I show is very inefficient but is simple and does work.

It's not kmh (kilometre hours), it's km/h (kilometres per hour).

Suggested

SimulatorFrame.java

package traffic;

import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;


public class SimulatorFrame extends JFrame {
    private enum State { stopped, paused, running }
    private State state = State.stopped;

    private final JButton btnStart = new JButton("Start");
    private final JButton btnPause = new JButton("Pause");

    private final Map<Light.State, Color> stateColors = Map.of(
        Light.State.started, Color.green,
        Light.State.stopSoon, Color.yellow,
        Light.State.stopped, Color.red
    );

    private final Timer t = new Timer(100, this::clockTicks);
    private Simulator sim;

    SimulatorFrame() {
        btnPause.setEnabled(false);
        btnPause.addActionListener(this::pauseClicked);
        btnStart.addActionListener(this::startClicked);

        JPanel jp = new JPanel();
        jp.setLayout(new BorderLayout());
        JPanel south = new JPanel();
        south.setLayout(new FlowLayout());
        south.add(btnStart);
        south.add(btnPause);
        jp.add(south, BorderLayout.SOUTH);
        jp.add(new Road(), BorderLayout.CENTER);
        getContentPane().add(jp);

        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setSize(1200, 700);
        setVisible(true);
    }

    private void pauseClicked(ActionEvent event) {
        state = switch (state) {
            case running -> {
                btnPause.setText("Continue");
                t.stop();
                yield State.paused;
            }
            case paused -> {
                btnPause.setText("Pause");
                t.start();
                yield State.running;
            }
            default -> throw new IllegalStateException();
        };
    }

    private void startClicked(ActionEvent event) {
        state = switch (state) {
            case stopped -> {
                btnStart.setText("Stop");
                btnPause.setEnabled(true);
                sim = new Simulator();
                t.start();
                yield State.running;
            }
            case running, paused -> {
                btnStart.setText("Start");
                btnPause.setText("Pause");
                btnPause.setEnabled(false);
                t.stop();
                sim = null;
                yield State.stopped;
            }
        };
    }

    private void clockTicks(ActionEvent event) {
        sim.tick();
        getContentPane().repaint();
    }

    private class Road extends JPanel {
        private record TextExtent(
            int y, int x1, int x2
        ) { }

        private static final int
            LIGHT_RADIUS = 20,
            LIGHT_DIAMETER = 2*LIGHT_RADIUS,
            LIGHT_Y = 350,
            LIGHT_STICK_HEIGHT = 60,
            CAR_Y = 400,
            CAR_TEXT_Y = 470;

        @Override
        protected void paintComponent(Graphics g) {
            super.paintComponent(g);
            if (sim == null) return;

            for (Light light: sim.getLights())
                paintLight(g, light);

            FontMetrics metrics = g.getFontMetrics();
            List<TextExtent> lines = new ArrayList<>();

            for (Car car: sim.getCars())
                paintCar(g, metrics, car, lines);
        }

        private void paintLight(Graphics g, Light light) {
            g.setColor(Color.BLACK);
            g.drawString(Integer.toString(light.getSeconds()), light.x, LIGHT_Y - 60);
            Color c = stateColors.get(light.getState());
            g.setColor(c);
            g.fillOval(light.x, LIGHT_Y, LIGHT_DIAMETER, LIGHT_DIAMETER);
            g.drawLine(
                light.x + LIGHT_RADIUS, LIGHT_Y + LIGHT_RADIUS,
                light.x + LIGHT_RADIUS, LIGHT_Y + LIGHT_STICK_HEIGHT
            );
        }

        private void paintCar(Graphics g, FontMetrics metrics, Car car, List<TextExtent> lines) {
            Color c = car.getColor();
            g.setColor(c);
            int x = (int)car.getX();
            g.fillRect(x, CAR_Y, 20, 20);

            String desc = "(%.0f, 0) %.2f km/h".formatted(
                car.getX(), car.getVelocity()
            );
            int w = metrics.stringWidth(desc),
                x2 = x + w,
                y;

            for (y = 0;; y++) {
                boolean found = false;
                for (TextExtent text: lines) {
                    if (
                        y == text.y
                            && x < text.x2
                            && x2 > text.x1
                    ) {
                        found = true;
                        break;
                    }
                }
                if (!found) break;
            }

            lines.add(new TextExtent(y, x, x2));

            g.setColor(Color.black);
            g.drawString(
                desc, x,
                CAR_TEXT_Y + metrics.getHeight()*y
            );
        }
    }

    public static void main(String[] args) {
        new SimulatorFrame();
    }
}

Simulator.java

package traffic;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Random;


public class Simulator {
    private final ArrayList<Light> lights = new ArrayList<>();
    private final ArrayList<Car> cars = new ArrayList<>();
    private final Random rand = new Random();
    private int ticks = 0;

    public Simulator() {
        int nLights = rand.nextInt(3, 5),
            distance = 1000 / nLights;

        for (int i = 0; i < nLights; i++) {
            int x = (i + 1) * distance;
            Light l = new Light(x, Light.State.getRandom(rand));
            lights.add(l);
        }

        for (int i = 0; i < 3; i++) {
            int x = rand.nextInt(500);
            cars.add(new Car(x, rand));
        }
    }

    public List<Light> getLights() {
        return Collections.unmodifiableList(lights);
    }

    public List<Car> getCars() {
        return Collections.unmodifiableList(cars);
    }

    private Light getLight(double x) {
        for (Light light: lights) {
            double diff = light.x - x;
            if (diff < 5 && diff > -1)
                return light;
        }
        return null;
    }

    public void tick() {
        boolean newCar = rand.nextInt(100) < 1;
        if (newCar) {
            int x = 50;
            cars.add(new Car(x, rand));
        }

        List<Car> toRemove = new ArrayList<>();
        for (Car car: cars) {
            double x = car.getX();
            Light light = getLight(x);
            car.setLight(light);
            car.tick();
            if (car.expired())
                toRemove.add(car);
        }
        for (Car car: toRemove)
            cars.remove(car);

        ticks++;
        if (ticks % 10 == 0) {
            for (Light light: lights)
                light.tick();
        }
    }
}

Light.java

package traffic;

import java.util.Random;

public class Light {
    public enum State {
        started, stopSoon, stopped;

        public State getNext() {
            int i = (ordinal() + 1) % values().length;
            return values()[i];
        }

        public static State getRandom(Random rand) {
            int i = rand.nextInt(values().length);
            return values()[i];
        }
    }

    private State state;
    private int seconds;
    public final int x;

    public Light(int x, State initialState) {
        this.x = x;
        setState(initialState);
    }

    public void tick() {
        if (seconds >= 1)
            seconds -= 1;
        else setState(state.getNext());
    }

    public int getSeconds() {
        return seconds;
    }

    public void setState(State state) {
        this.state = state;

        seconds = switch (state) {
            case stopped -> 15;
            case started -> 10;
            default -> 3;
        };
    }

    public State getState() {
        return state;
    }

}

Car.java

package traffic;

import java.awt.Color;
import java.util.Random;


public class Car {
    private static final Color[] colors = {
        Color.blue, Color.lightGray, Color.cyan, Color.magenta
    };
    private final double velocity;
    private final Color color;
    private double x;
    private Light light;

    public Car(double x, double velocity, Color color) {
        this.x = x;
        this.velocity = velocity;
        this.color = color;
    }

    public Car(double x, Random rand) {
        this(x, randomVelocity(rand), randomColor(rand));
    }

    private static double randomVelocity(Random rand) {
        return rand.nextDouble(0, 25);
    }

    private static Color randomColor(Random rand) {
        return colors[rand.nextInt(colors.length)];
    }

    public double getX() {
        return x;
    }

    public double getVelocity() {
        if (!isStopped()) {
            return velocity;
        }
        return 0;
    }

    public Color getColor() {
        return color;
    }

    private boolean isStopped() {
        return light != null && light.getState() == Light.State.stopped;
    }

    public void tick() {
        x += getVelocity() * 0.1;
    }

    public void setLight(Light light) {
        this.light = light;
    }

    public boolean expired() {
        return x >= 1200;
    }
}

Output