How should I refactor my code to increase its modularity or reduce complexity?

Question

I believe this set of code is very repetitive in a way where every method does almost the same task but it is repeated multiple times. This is what the code looks like and its main purpose is to display the intended design on the simulator:

private void configStart() {
 button1 = new JButton("start");
 button1.setBackground(Color.lightGray);
 button1.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 if(vehicle == null) {
 int selectedIndex = combobox.getSelectedIndex();
 String vehicleName = vehicles[selectedIndex];
 initialiseVehicle(vehicleName);
 speedlabel.setText(vehicle.printSpeed());
 }
 if(simulationPane !=null) {
 frame.remove(simulationPane);
 }
 accelerate = false;
 decelerate = false;
 cruise = false;
 stop = false;
 button1.setBackground(Color.GREEN);
 button2.setBackground(Color.lightGray);
 button3.setBackground(Color.lightGray);
 button4.setBackground(Color.lightGray);
 button5.setBackground(Color.lightGray);
 simulationPane = new Simulator();
 frame.add(simulationPane,BorderLayout.CENTER);
 frame.revalidate();
 frame.repaint();
 }
 });
}
private void configAccelerate() {
 button2 = new JButton("accelerate");
 button2.setBackground(Color.lightGray);
 button2.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 accelerate = true;
 decelerate = false;
 cruise = false;
 stop = false;
 button1.setBackground(Color.lightGray);
 button2.setBackground(Color.green);
 button3.setBackground(Color.lightGray);
 button4.setBackground(Color.lightGray);
 button5.setBackground(Color.lightGray);
 Thread thread = new Thread(){
 public void run(){
 try {
 while(accelerate) {
 Thread.sleep(2 * 1000);
 if(currentvelocity<=maximumvelocity) {
 currentvelocity = currentvelocity +1;
 vehicle.setCurrentSpeed(currentvelocity);
 speedlabel.setText(vehicle.printSpeed());
 simulationPane.updateTimer();
 } 
 }
 }
 catch (InterruptedException e) {
 e.printStackTrace();
 } 
 }
 };
 thread.start();
 } 
 });
}
private void configCruise() {
 button3 = new JButton("cruise");
 button3.setBackground(Color.lightGray);
 button3.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 accelerate = false;
 decelerate = false;
 cruise = true;
 stop = false;
 button1.setBackground(Color.lightGray);
 button2.setBackground(Color.lightGray);
 button3.setBackground(Color.green);
 button4.setBackground(Color.lightGray);
 button5.setBackground(Color.lightGray);
 } 
 });
}
private void configDecelerate() {
 button4 = new JButton("decelerate");
 button4.setBackground(Color.lightGray);
 button4.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 accelerate = false;
 decelerate = true;
 cruise = false;
 stop = false;
 button1.setBackground(Color.lightGray);
 button2.setBackground(Color.lightGray);
 button3.setBackground(Color.lightGray);
 button4.setBackground(Color.green);
 button5.setBackground(Color.lightGray);
 
 Thread thread = new Thread(){
 public void run(){
 try {
 while(decelerate) {
 Thread.sleep(2 * 1000);
 if(currentvelocity >1) {
 currentvelocity = currentvelocity -1;
 vehicle.setCurrentSpeed(currentvelocity);
 speedlabel.setText(vehicle.printSpeed());
 simulationPane.updateTimer();
 } 
 }
 }
 catch (InterruptedException e) {
 e.printStackTrace();
 } 
 }
 };
 thread.start();
 } 
 });
}
private void configStop() {
 button5 = new JButton("stop");
 button5.setBackground(Color.lightGray);
 button5.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 accelerate = false;
 decelerate = false;
 cruise = false;
 stop = true;
 button1.setBackground(Color.lightGray);
 button2.setBackground(Color.lightGray);
 button3.setBackground(Color.lightGray);
 button4.setBackground(Color.lightGray);
 button5.setBackground(Color.green);
 
 currentvelocity = 1;
 vehicle.setCurrentSpeed(currentvelocity);
 speedlabel.setText(vehicle.printSpeed());
 simulationPane.updateTimer();
 } 
 });
}

I would like to ask how it can be refactored to increase its modularity or reduce complexity. Thanks!

• BCdotWEB: The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?.
• BCdotWEB: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Please follow the tour, and read "What topics can I ask about here?", "How do I ask a good question?" and "What types of questions should I avoid asking?".

Answer 1

Since I cannot compile this code, there might be small mistakes in my code. I'm not writing Java for a long time. Hope it helps.

I see your buttons work as a state machine, so I created a enum called ButtonState to represent each state:

enum ButtonState {
 Start,
 Accelerate,
 Cruise,
 Decelerate,
 Stop;
}

I see you have a bunch of buttons named like button1, button2 ... etc. Don't name your variables with numbers. Instead name them with their associated actions, like startButton, accelerateButton etc. If you have a cluster of variables with same type, it indicates that you can store them in a container. I used array in this example. Since there is a button for each state we know the size of buttons:

JButton[] buttons = new JButton[ButtonState.values().length()];.

To initialize buttons we can iterate through states:

for (ButtonState state : ButtonState.values()) {
 JButton button = new JButton(state.name().toLowerCase()); // button name
 button.setBackground(Color.lightGray); // button color
 button.addActionListener(...); // button action
 buttons[state.ordinal()] = button;
}

You can get buttons with state ordinals, let's say you want to get cruise button, you can get it like buttons[ButtonState.Cruise.ordinal()].

I see you have a boolean variables for each state(except start). Let's assume you also have bool start to complete pattern. I see only one of them gets true and others get false each time. So instead of having booleans for each of state, we can have only one state that represents which state is true. In this case it is ButtonState state.

Before:

accelerate = false;
decelerate = true;
cruise = false;
stop = false;

After:

state = ButtonState.Decelerate;

Before:

if (decelerate)

After:

if (state == ButtonState.Decelerate)

Now we can get rid of most repetitive pattern in your code: Setting a state boolean and highlighting the corresponding button. Let's gather it in a function:

private void setState(ButtonState state) {
 this.buttons[this.state.ordinal()].setBackground(Color.lightGray); // fade highlighted button
 this.buttons[state.ordinal()].setBackground(Color.green); // highlight new state button
 this.state = state; // set state
}

Before:

accelerate = true;
decelerate = false;
cruise = false;
stop = false;
button1.setBackground(Color.lightGray);
button2.setBackground(Color.green);
button3.setBackground(Color.lightGray);
button4.setBackground(Color.lightGray);
button5.setBackground(Color.lightGray);

After:

this.setState(ButtonState.Accelerate);

Bonus: In a scenario where multiple states combined, like accelerating and cruising at the same time, enums with bit flags can be used.

Refactored Code

enum ButtonState {
 Start,
 Accelerate,
 Cruise,
 Decelerate,
 Stop;
}
private JButton[] buttons;
private ButtonState state;
private void setState(ButtonState state) {
 this.buttons[this.state.ordinal()].setBackground(Color.lightGray);
 this.buttons[state.ordinal()].setBackground(Color.green);
 this.state = state;
}
private void configButtons() {
 this.buttons = new JButton[ButtonState.values().length()];
 
 for (ButtonState state : ButtonState.values()) {
 JButton button = new JButton(state.name().toLowerCase());
 button.setBackground(Color.lightGray);
 
 switch(state) {
 case Start: 
 button.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 start(e);
 }
 });
 break;
 
 case Accelerate: 
 button.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 accelerate(e);
 }
 });
 break;
 
 case Cruise: return
 button.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 cruise(e);
 }
 });
 break;
 
 case Decelerate:
 button.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 decelerate(e);
 }
 });
 break;
 
 case Stop:
 button.addActionListener(new ActionListener() {
 @Override
 public void actionPerformed(ActionEvent e) {
 stop(e);
 }
 });
 break;
 }
 
 this.buttons[state.ordinal()] = button;
 }
}
private void start(ActionEvent e) {
 if(vehicle == null) {
 int selectedIndex = combobox.getSelectedIndex();
 String vehicleName = vehicles[selectedIndex];
 initialiseVehicle(vehicleName);
 speedlabel.setText(vehicle.printSpeed());
 }
 
 if(simulationPane !=null) {
 frame.remove(simulationPane);
 }
 
 this.setState(ButtonState.Start);
 simulationPane = new Simulator();
 frame.add(simulationPane,BorderLayout.CENTER);
 frame.revalidate();
 frame.repaint();
}
private void accelerate(ActionEvent e) {
 this.setState(ButtonState.Accelerate);
 Thread thread = new Thread(){
 public void run(){
 try {
 while(accelerate) {
 Thread.sleep(2 * 1000);
 if(currentvelocity<=maximumvelocity) {
 currentvelocity = currentvelocity +1;
 vehicle.setCurrentSpeed(currentvelocity);
 speedlabel.setText(vehicle.printSpeed());
 simulationPane.updateTimer();
 } 
 }
 }
 catch (InterruptedException e) {
 e.printStackTrace();
 } 
 }
 };
 thread.start();
}
private void cruise(ActionEvent e) {
 this.setState(ButtonState.Cruise);
}
private void decelerate(ActionEvent e) {
 this.setState(ButtonState.Decelerate);
 
 Thread thread = new Thread(){
 public void run(){
 try {
 while(decelerate) {
 Thread.sleep(2 * 1000);
 if(currentvelocity >1) {
 currentvelocity = currentvelocity -1;
 vehicle.setCurrentSpeed(currentvelocity);
 speedlabel.setText(vehicle.printSpeed());
 simulationPane.updateTimer();
 } 
 }
 }
 catch (InterruptedException e) {
 e.printStackTrace();
 } 
 }
 };
 thread.start();
}
private void stop(ActionEvent e) {
 this.setState(ButtonState.Stop);
 
 currentvelocity = 1;
 vehicle.setCurrentSpeed(currentvelocity);
 speedlabel.setText(vehicle.printSpeed());
 simulationPane.updateTimer();
}

Happy Coding;P

• greybeard: (Since I cannot compile this code more and more online compilation services are available (e.g., tutorialspoint.com).)
• Toby Speight: Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer.
• Kao: @greybeard I don't know how rest of class looks like, there are lots of members I have no idea about them, plus I have to import packages for JButton etc. More explanatory edit incoming.
• mdfst13: @greybeard Have you even looked at the original code? I encourage you to actually try compiling it on Tutorialspoint. It just gives a continuous stream of errors. And even if those were fixed, it's a GUI. While it is barely possible to compile a GUI online (see repl.it ), Tutorialspoint can't. I realize that you may be busy. But if you don't have time to glance at the code, perhaps you shouldn't have time to post. As is, you have simply been gratuitously rude.