2
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Not sure whether this counts as an answer but I think there is strong reason to switch to JavaFX instead of using Swing. See here. \$\endgroup\$ – Dair Jan 8 at 4:47
  • \$\begingroup\$ Also, I haven't used Swing, but it seems part of the reason you should use JavaFX is that it better supports the MVC paradigm. In particular, you can utilize FXML/CSS to design the UI and let the logic be controlled in Java. This isn't particularly in depth since I don't do much Java development, but the general rule of thumb for GUI applications is to not write static UIs in Java/C/etc. and use a markup-esque language to do so instead. \$\endgroup\$ – Dair Jan 8 at 4:52
2
\$\begingroup\$

In no particular order:

  • Trust the math. When you test for value > 250 || value < 500 you already know that value is greater than 250 (otherwise, the code would go into if clause). Testing for value < 500 suffices.

    BTW, what a background color would be if value is 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 Color value. Assign it in the ifs, 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 same actionPerformed for each of them.

    Speaking of which, why jRadioButton1.isSelected() case calls t1.start() twice?

  • It feels creepy that jButton1ActionPerformed creates new App(). An App should be created once. I am afraid there is a big design problem here.

  • sum = value + value / 10 is very much a mistake. Even sum = sum + value / 10 does not compute the correct average.

  • I see reporting the (incorrect) average, and the min time. I don't see reporting max time.

\$\endgroup\$
  • \$\begingroup\$ what exactly is the design problem and how can i fix it ? can you please explain in more details? i was trying to call the method when you click the button \$\endgroup\$ – Leon Bogod Jan 8 at 20:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.