Problem
I’m still a freshman and this one of my first projects I made using Java and Swing.
The program is a simple League of Legends multi-threaded ping checker that pings the server 10 times to get an average ping and determine the max and minimum ranges.
How can I make my code cleaner instead of writing so many if statements? Any suggestions or tips are greatly appreciated.
To check your ping you can download the software here.
import java.awt.Color;
import javax.swing.text.AttributeSet;
import javax.swing.text.SimpleAttributeSet;
import javax.swing.text.StyleConstants;
import javax.swing.text.StyleContext;
import java.net.InetAddress;
import java.util.GregorianCalendar;
import java.util.*;
import javax.swing.JProgressBar;
/**
*
* @author User
*/
public class App {
public static void sendPingRequest2(String ipAddress, javax.swing.JTextPane jTextPane1, javax.swing.JTextPane jTextPane2, javax.swing.JTextField jTextField1) {
try {
for (int i = 0; i < 10; i++) {
InetAddress inet = InetAddress.getByName(ipAddress);
long finish = 0;
long start = new GregorianCalendar().getTimeInMillis();
System.out.println("Sending Ping Request to " + ipAddress);
long sum = 0;
List < Long > list = new ArrayList < > ();
if (inet.isReachable(5000)) {
finish = new GregorianCalendar().getTimeInMillis();
long value = finish - start;
list.add(value);
long lowest = Collections.min(list);
System.out.println("value" + lowest);
sum = value + value / 10;
if (value < 250) {
jTextPane1.setText("Ping RTT: " + value + "ms");
jTextPane2.setText("average ping: " + sum + "ms");
jTextField1.setText("" + lowest + "ms");
jTextPane1.setBackground(Color.green);
jTextPane2.setForeground(Color.green);
jTextField1.setBackground(Color.green);
} else if (value > 250 || value < 500) {
jTextPane1.setText("Ping RTT: " + value + "ms");
jTextPane2.setText("average ping: " + sum + "ms");
jTextField1.setText("" + lowest + "ms");
jTextPane1.setBackground(Color.yellow);
jTextPane2.setBackground(Color.yellow);
jTextField1.setBackground(Color.yellow);
} else {
jTextPane1.setText("Ping RTT: " + value + "ms");
jTextPane2.setText(" " + sum + "ms");
jTextField1.setText("" + lowest + "ms");
jTextPane1.setBackground(Color.red);
jTextPane2.setBackground(Color.red);
jTextField1.setBackground(Color.red);
}
} else {
System.out.println(ipAddress + " NOT reachable.");
}
}
} catch (Exception e) {
System.out.println("Exception:" + e.getMessage());
}
}
public static void main(String[] args) {
App m = new App();
NewJFrame jf = new NewJFrame();
}
void sendPingRequest(String string) {
throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.
}
}
Gui.java
package selenium;
import java.net.InetAddress;
import java.util.GregorianCalendar;
import javax.swing.JProgressBar;
import javax.swing.JTextField;
/**
*
* @author User
public class NewJFrame extends javax.swing.JFrame {
* Creates new form NewJFrame
*/
public NewJFrame() {
initComponents();
}
private void jButton1ActionPerformed(java.awt.event.ActionEvent evt) {
// TODO add your handling code here:
App m = new App();
if (jRadioButton1.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.141.3", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
t1.start();
}
if (jRadioButton2.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.156.1", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
}
if (jRadioButton3.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.142.3", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
}
if (jRadioButton4.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.136.3", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
}
if (jRadioButton5.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.131.3", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
}
}
private void jTextField1ActionPerformed(java.awt.event.ActionEvent evt) {
// TODO add your handling code here:
}
private void jRadioButton2ActionPerformed(java.awt.event.ActionEvent evt) {
// TODO add your handling code here:
}
private void jButton1ActionPerformed(java.awt.event.ActionEvent evt) {
// TODO add your handling code here:
App m = new App();
if (jRadioButton1.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.141.3", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
t1.start();
}
if (jRadioButton2.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.156.1", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
}
if (jRadioButton3.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.142.3", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
}
if (jRadioButton4.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.136.3", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
}
if (jRadioButton5.isSelected()) {
Thread t1 = new Thread(new progress(jProgressBar1));
t1.start();
Thread t2 = new Thread(new Runnable() {
@Override
public void run() {
m.sendPingRequest2("104.160.131.3", jTextPane1, jTextPane2, jTextField1); // Insert some method call here.
}
});
t2.start();
}
}
private void jTextField1ActionPerformed(java.awt.event.ActionEvent evt) {
// TODO add your handling code here:
}
private void jRadioButton2ActionPerformed(java.awt.event.ActionEvent evt) {
// TODO add your handling code here:
}
constants.java
public enum Constants {
NA ("104.160.131.3"), EUW("104.160.141.3"),EUNE("104.160.142.3"),KOR("104.160.156.1"),LAN("104.160.136.3");
private String ip;
Constants(String ip){
this.ip = ip;
}
public String getIp(){
return ip;
}
}
Solution
In no particular order:
-
Trust the math. When you test for
value > 250 || value < 500you already know thatvalueis greater than 250 (otherwise, the code would go intoifclause). Testing forvalue < 500suffices.BTW, what a background color would be if
valueis exactly 250? -
DRY #1. The block of code
jTextPane1.setText("Ping RTT: " + value + "ms"); jTextPane2.setText("average ping: " + sum + "ms"); jTextField1.setText("" + lowest + "ms"); jTextPane1.setBackground(Color.green); jTextPane2.setForeground(Color.green); jTextField1.setBackground(Color.green);is repeated thrice. The only difference is
Colorvalue. Assign it in theifs, and factor the rest out:if (value < 250) { color = Color.green; } else if (value < 500) { color = Color.yellow; } else { color = Color.red; } jTextPane1.setText("Ping RTT: " + value + "ms"); jTextPane2.setText("average ping: " + sum + "ms"); jTextField1.setText("" + lowest + "ms"); jTextPane1.setBackground(color); jTextPane2.setForeground(color); jTextField1.setBackground(color); -
DRY #2. Ditto for
if (jRadioButtonX.isSelected())cases. The only difference is the IP you are going to ping. Associate the IP with the button, and use the sameactionPerformedfor each of them.Speaking of which, why
jRadioButton1.isSelected()case callst1.start()twice? -
It feels creepy that
jButton1ActionPerformedcreatesnew App(). AnAppshould be created once. I am afraid there is a big design problem here. -
sum = value + value / 10is very much a mistake. Evensum = sum + value / 10does not compute the correct average. -
I see reporting the (incorrect) average, and the min time. I don’t see reporting max time.