3
\$\begingroup\$

I wrote a Java program with a Swing UI that calculates prime numbers. The primes are displayed into a listbox. I used a few optimizations to generate the primes faster. I’m using the NetBeans IDE, so there’s a lot of generated Swing code.

Currently, the program finds as many primes as the user has input into a spinner box. Ideally, I’d like to change the program to include just a simple start/stop button, and the primes are displayed in the list realtime, but that’s for a another day.

The program has 2 classes.

PrimeNumberCalculator.java

import javax.swing.DefaultListModel;
import java.text.NumberFormat;
import javax.swing.JOptionPane;
import primenumber.PrimeCalculator;

public class PrimeNumberCalculator extends javax.swing.JFrame {

    private int currentIteration = 2;   // Stores the next number to be checked

    public DefaultListModel numListModel = new DefaultListModel();  // List model for the jList
    NumberFormat numberFormatter = NumberFormat.getNumberInstance();

    public PrimeNumberCalculator() {
        initComponents();   // Do not delete
        this.setLocationRelativeTo(null);   // Center the window
    }

    @SuppressWarnings("unchecked")
    // <editor-fold defaultstate="collapsed" desc="Generated Code">                          
    private void initComponents() {

        StartButton = new javax.swing.JButton();
        jScrollPane1 = new javax.swing.JScrollPane();
        PrimesList = new javax.swing.JList();
        StatusBar = new javax.swing.JPanel();
        StatusLabel = new javax.swing.JLabel();
        StatusNumber = new javax.swing.JLabel();
        PrimeAmountSpinner = new javax.swing.JSpinner();
        ClearButton = new javax.swing.JButton();

        setDefaultCloseOperation(javax.swing.WindowConstants.EXIT_ON_CLOSE);
        setTitle("Prime Number Calculator");
        setMinimumSize(new java.awt.Dimension(250, 250));
        setSize(new java.awt.Dimension(300, 400));

        StartButton.setFont(StartButton.getFont().deriveFont(StartButton.getFont().getStyle() | java.awt.Font.BOLD));
        StartButton.setText("Calculate");
        StartButton.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(java.awt.event.ActionEvent evt) {
                StartButtonActionPerformed(evt);
            }
        });

        jScrollPane1.setVerticalScrollBarPolicy(javax.swing.ScrollPaneConstants.VERTICAL_SCROLLBAR_ALWAYS);

        PrimesList.setFont(PrimesList.getFont());
        PrimesList.setModel(numListModel);
        jScrollPane1.setViewportView(PrimesList);

        StatusLabel.setFont(StatusLabel.getFont());
        StatusLabel.setText("Checked: ");
        StatusLabel.setCursor(new java.awt.Cursor(java.awt.Cursor.HAND_CURSOR));

        StatusNumber.setFont(StatusNumber.getFont());
        StatusNumber.setText("-");

        javax.swing.GroupLayout StatusBarLayout = new javax.swing.GroupLayout(StatusBar);
        StatusBar.setLayout(StatusBarLayout);
        StatusBarLayout.setHorizontalGroup(
            StatusBarLayout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
            .addGroup(StatusBarLayout.createSequentialGroup()
                .addContainerGap()
                .addComponent(StatusLabel)
                .addGap(0, 0, 0)
                .addComponent(StatusNumber)
                .addContainerGap(javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE))
        );
        StatusBarLayout.setVerticalGroup(
            StatusBarLayout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
            .addGroup(javax.swing.GroupLayout.Alignment.TRAILING, StatusBarLayout.createSequentialGroup()
                .addContainerGap()
                .addGroup(StatusBarLayout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE)
                    .addComponent(StatusLabel)
                    .addComponent(StatusNumber))
                .addContainerGap())
        );

        PrimeAmountSpinner.setFont(PrimeAmountSpinner.getFont());
        PrimeAmountSpinner.setModel(new javax.swing.SpinnerNumberModel(Integer.valueOf(1), Integer.valueOf(1), null, Integer.valueOf(1)));
        PrimeAmountSpinner.setToolTipText("Prime numbers to generate");

        ClearButton.setFont(ClearButton.getFont());
        ClearButton.setText("Clear");
        ClearButton.setEnabled(false);
        ClearButton.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(java.awt.event.ActionEvent evt) {
                ClearButtonActionPerformed(evt);
            }
        });

        javax.swing.GroupLayout layout = new javax.swing.GroupLayout(getContentPane());
        getContentPane().setLayout(layout);
        layout.setHorizontalGroup(
            layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
            .addGroup(layout.createSequentialGroup()
                .addContainerGap()
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                    .addComponent(jScrollPane1, javax.swing.GroupLayout.PREFERRED_SIZE, 0, Short.MAX_VALUE)
                    .addGroup(layout.createSequentialGroup()
                        .addComponent(PrimeAmountSpinner, javax.swing.GroupLayout.DEFAULT_SIZE, 94, Short.MAX_VALUE)
                        .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED)
                        .addComponent(StartButton, javax.swing.GroupLayout.PREFERRED_SIZE, 106, javax.swing.GroupLayout.PREFERRED_SIZE)
                        .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED)
                        .addComponent(ClearButton)))
                .addContainerGap())
            .addComponent(StatusBar, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE)
        );
        layout.setVerticalGroup(
            layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
            .addGroup(layout.createSequentialGroup()
                .addContainerGap()
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE)
                    .addComponent(StartButton)
                    .addComponent(PrimeAmountSpinner, javax.swing.GroupLayout.PREFERRED_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.PREFERRED_SIZE)
                    .addComponent(ClearButton))
                .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED)
                .addComponent(jScrollPane1, javax.swing.GroupLayout.DEFAULT_SIZE, 330, Short.MAX_VALUE)
                .addGap(0, 0, 0)
                .addComponent(StatusBar, javax.swing.GroupLayout.PREFERRED_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.PREFERRED_SIZE))
        );

        pack();
    }// </editor-fold>                        

    private void calculatePrimeNumbers() {
        int primesToFind = Integer.parseInt(PrimeAmountSpinner.getValue().toString());
        int primesFound = 0;    // The primes that have been found in this test set

        try {

            while(primesFound < primesToFind) {
                if ( PrimeCalculator.isPrime(currentIteration) ) {
                    numListModel.addElement( numberFormatter.format(currentIteration) );    // Add to the jList
                    primesFound++;
                }
                currentIteration++; // Increase the currentIteration counter to the next number to be checked
            }

        } catch (java.lang.OutOfMemoryError e) {
            // To prevent out of memory errors if the prime cache grows too large
            clearAll();
            JOptionPane.showMessageDialog(this, "Out of memory. The program will now reset.", "Critical Error", JOptionPane.ERROR_MESSAGE);
        }

    }

    private void clearAll() {
        // Reset variables and values
        currentIteration = 2;
        StatusNumber.setText("-");
        ClearButton.setEnabled(false);
        // Clear the prime cache and list
        PrimeCalculator.clearCache();
        numListModel.clear();
        System.gc();
    }

    private void StartButtonActionPerformed(java.awt.event.ActionEvent evt) {                                            
        calculatePrimeNumbers();
        StatusNumber.setText( numberFormatter.format(currentIteration-1) );
        ClearButton.setEnabled(true);
    }                                           

    private void ClearButtonActionPerformed(java.awt.event.ActionEvent evt) {                                            
        clearAll();
        PrimeAmountSpinner.requestFocusInWindow();
    }                                           

    /**
     @param args the command line arguments
     */
    public static void main(String args[]) {

        //<editor-fold defaultstate="collapsed" desc=" Look and feel setting code">
        try {
            javax.swing.UIManager.setLookAndFeel( javax.swing.UIManager.getSystemLookAndFeelClassName() );
        } catch (ClassNotFoundException ex) {
            java.util.logging.Logger.getLogger(PrimeNumberCalculator.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        } catch (InstantiationException ex) {
            java.util.logging.Logger.getLogger(PrimeNumberCalculator.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        } catch (IllegalAccessException ex) {
            java.util.logging.Logger.getLogger(PrimeNumberCalculator.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        } catch (javax.swing.UnsupportedLookAndFeelException ex) {
            java.util.logging.Logger.getLogger(PrimeNumberCalculator.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        }
        //</editor-fold>

        // Create and display the form
        java.awt.EventQueue.invokeLater(new Runnable() {
            public void run() {
                new PrimeNumberCalculator().setVisible(true);
            }
        });
    }

    // Variables declaration - do not modify                     
    private javax.swing.JButton ClearButton;
    private javax.swing.JSpinner PrimeAmountSpinner;
    private javax.swing.JList PrimesList;
    private javax.swing.JButton StartButton;
    private javax.swing.JPanel StatusBar;
    private javax.swing.JLabel StatusLabel;
    private javax.swing.JLabel StatusNumber;
    private javax.swing.JScrollPane jScrollPane1;
    // End of variables declaration                   

}

PrimeCalculator.java

import java.util.ArrayList;

public class PrimeCalculator {

    public static ArrayList<Integer> primeNumberCache = new ArrayList<>();

    public static boolean isPrime(int primeToCheck) {

        boolean isPrime = true;
        // Set the isPrime flag for this number to true initially, and set it to false later if the number is not prime

            // Loop that will test the prime number against prime factors
            for (int currentTest : primeNumberCache) {
                /*
                This code determines if the number is prime or not
                The possible factors of a number can be reduced to prime numbers lower than the number
                The second check tests the current number against all previous prime numbers
                Because of factor pairs, only factors less than the square root of the number need to be checked
                The first check stops the tests early if all primes that need to be checked have been
                */

                double sqrtOfCurrentIteration = Math.sqrt(primeToCheck);    // Cached for optimisation
                if ( currentTest > sqrtOfCurrentIteration ) {
                    break;  // For optimisation, break out of the loop if all important primes have been checked
                }

                //Test if the current number is evenly divisible (not prime)
                if ( primeToCheck % currentTest == 0 ) {
                    isPrime = false;
                    break;  // For optimisation, break out of the loop if the number is not prime
                }

            }

        if (isPrime == true) {
            primeNumberCache.add(primeToCheck); // Add the found prime number to the lists
            return true;
        } else {
            return false;
        }

    }

    public static int getPrimeCount() {
        return primeNumberCache.size();
    }

    public static void clearCache() {
        primeNumberCache.clear();
    }

}
\$\endgroup\$
2
\$\begingroup\$

PrimeNumberCalculator and PrimeCalculator; those names are very similar. I can't tell what they do differently just from the name. You should name them based on what they do. PrimeNumberCalculator would be better named something along the lines of PrimeNumberGUI.


I'm just going to review PrimeNumberCalculator.


javax.swing.JButton ...

Please stop using fully-qualified names. You have such a large number of them that the code is hard to read. Instead, import them (NetBeans has a shortcut to automatically import such classes). Also, it's good to organize your imports into some logical order, but don't worry about it if the IDE put them in that order anyway.

import javax.swing.DefaultListModel;
import javax.swing.JOptionPane;
import javax.swing.JButton;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JFrame; // and so on

import java.text.NumberFormat;

import primenumber.PrimeCalculator;

public class PrimeNumberCalculator extends javax.swing.JFrame {

It is considered bad to extend JFrame. For most purposes, you don't even have to extend Swing components. The only time you'd need to is if you want a custom paint method, so just stay away from it. Favor composition over extending.


This is one function:

@SuppressWarnings("unchecked")
// <editor-fold defaultstate="collapsed" desc="Generated Code">                       
private void initComponents() {

    StartButton = new javax.swing.JButton();
    jScrollPane1 = new javax.swing.JScrollPane();
    PrimesList = new javax.swing.JList();
    StatusBar = new javax.swing.JPanel();
    StatusLabel = new javax.swing.JLabel();
    StatusNumber = new javax.swing.JLabel();
    PrimeAmountSpinner = new javax.swing.JSpinner();
    ClearButton = new javax.swing.JButton();

    setDefaultCloseOperation(javax.swing.WindowConstants.EXIT_ON_CLOSE);
    setTitle("Prime Number Calculator");
    setMinimumSize(new java.awt.Dimension(250, 250));
    setSize(new java.awt.Dimension(300, 400));

    StartButton.setFont(StartButton.getFont().deriveFont(StartButton.getFont().getStyle() | java.awt.Font.BOLD));
    StartButton.setText("Calculate");
    StartButton.addActionListener(new java.awt.event.ActionListener() {
        public void actionPerformed(java.awt.event.ActionEvent evt) {
            StartButtonActionPerformed(evt);
        }
    });

    jScrollPane1.setVerticalScrollBarPolicy(javax.swing.ScrollPaneConstants.VERTICAL_SCROLLBAR_ALWAYS);

    PrimesList.setFont(PrimesList.getFont());
    PrimesList.setModel(numListModel);
    jScrollPane1.setViewportView(PrimesList);

    StatusLabel.setFont(StatusLabel.getFont());
    StatusLabel.setText("Checked: ");
    StatusLabel.setCursor(new java.awt.Cursor(java.awt.Cursor.HAND_CURSOR));

    StatusNumber.setFont(StatusNumber.getFont());
    StatusNumber.setText("-");

    javax.swing.GroupLayout StatusBarLayout = new javax.swing.GroupLayout(StatusBar);
    StatusBar.setLayout(StatusBarLayout);
    StatusBarLayout.setHorizontalGroup(
        StatusBarLayout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
        .addGroup(StatusBarLayout.createSequentialGroup()
            .addContainerGap()
            .addComponent(StatusLabel)
            .addGap(0, 0, 0)
            .addComponent(StatusNumber)
            .addContainerGap(javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE))
    );
    StatusBarLayout.setVerticalGroup(
        StatusBarLayout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
        .addGroup(javax.swing.GroupLayout.Alignment.TRAILING, StatusBarLayout.createSequentialGroup()
            .addContainerGap()
            .addGroup(StatusBarLayout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE)
                .addComponent(StatusLabel)
                .addComponent(StatusNumber))
            .addContainerGap())
    );

    PrimeAmountSpinner.setFont(PrimeAmountSpinner.getFont());
    PrimeAmountSpinner.setModel(new javax.swing.SpinnerNumberModel(Integer.valueOf(1), Integer.valueOf(1), null, Integer.valueOf(1)));
    PrimeAmountSpinner.setToolTipText("Prime numbers to generate");

    ClearButton.setFont(ClearButton.getFont());
    ClearButton.setText("Clear");
    ClearButton.setEnabled(false);
    ClearButton.addActionListener(new java.awt.event.ActionListener() {
        public void actionPerformed(java.awt.event.ActionEvent evt) {
            ClearButtonActionPerformed(evt);
        }
    });

    javax.swing.GroupLayout layout = new javax.swing.GroupLayout(getContentPane());
    getContentPane().setLayout(layout);
    layout.setHorizontalGroup(
        layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
        .addGroup(layout.createSequentialGroup()
            .addContainerGap()
            .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                .addComponent(jScrollPane1, javax.swing.GroupLayout.PREFERRED_SIZE, 0, Short.MAX_VALUE)
                .addGroup(layout.createSequentialGroup()
                    .addComponent(PrimeAmountSpinner, javax.swing.GroupLayout.DEFAULT_SIZE, 94, Short.MAX_VALUE)
                    .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED)
                    .addComponent(StartButton, javax.swing.GroupLayout.PREFERRED_SIZE, 106, javax.swing.GroupLayout.PREFERRED_SIZE)
                    .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED)
                    .addComponent(ClearButton)))
            .addContainerGap())
        .addComponent(StatusBar, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE)
    );
    layout.setVerticalGroup(
        layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
        .addGroup(layout.createSequentialGroup()
            .addContainerGap()
            .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE)
                .addComponent(StartButton)
                .addComponent(PrimeAmountSpinner, javax.swing.GroupLayout.PREFERRED_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.PREFERRED_SIZE)
                .addComponent(ClearButton))
            .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED)
            .addComponent(jScrollPane1, javax.swing.GroupLayout.DEFAULT_SIZE, 330, Short.MAX_VALUE)
            .addGap(0, 0, 0)
            .addComponent(StatusBar, javax.swing.GroupLayout.PREFERRED_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.PREFERRED_SIZE))
    );

    pack();
}// </editor-fold>

This is a big problem. This function is way too big. Break it up. Have different initialize functions (for the different sections of GUI; I didn't read the whole function to determine good sections).

Additionally, I have scanned the code several times, and I still don't know why you have @SuppressWarnings("unchecked") on the method. You should almost never have to use this; there are almost always ways to make this unnecessary. This means that you are using some generified type without a type parameter (e.g. ArrayList instead of ArrayList<Integer>). It is likely that that is unnecessary.


private javax.swing.JButton ClearButton;
private javax.swing.JSpinner PrimeAmountSpinner;
private javax.swing.JList PrimesList;
private javax.swing.JButton StartButton;
private javax.swing.JPanel StatusBar;
private javax.swing.JLabel StatusLabel;
private javax.swing.JLabel StatusNumber;

None of these variables should have a name beginning with a capital letter. Java convention is camelCase, not PascalCase. The same thing for your function names:

private void ClearButtonActionPerformed(java.awt.event.ActionEvent evt) {

The only things which should be PascalCase are class names.

Additionally, most of these variables aren't used externally from the init method. I would recommend keeping them as local variables to that method (after you break up that method, keep them as local variables to the specific sub-methods).


I'm not sure how you are arranging your GUI, but I'm imagining something like this:

+----------JFrame------------+
|+---------JPanel-----------+|
||                          ||
|| "Label of some sorts"    ||
|| [Field for prime number] ||
|| (btn)     (clrBtn)       ||
||                          ||
|| - list                   ||
|| - of                     ||
|| - sorts                  ||
|+--------------------------+|
+----------------------------+

So I would expect to see something like this in your code somewhere:

JFrame frame = new JFrame();
JPanel panel = new JPanel();

panel.add(myLabel);
panel.add(primeField);
panel.add(btn);
panel.add(clrBtn);
panel.add(list);

frame.add(panel);

Instead, the components are buried inside that scary method.


The absolute last thing that should be done is the following:

frame.pack();
frame.setLocationRelativeTo(null); // alternatively, frame.setLocationByPlatform(true);
frame.setVisible(true);

This ensures the frame is properly sized. However, I saw this in that method:

setMinimumSize(new java.awt.Dimension(250, 250));
setSize(new java.awt.Dimension(300, 400));

Don't do this. You should never need to set explicit sizes. frame.pack() should take care of the sizing for you.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the feedback. I agree that the code you talked about is horrendous, but much of that code is automatically generated by NetBean’s GUI builder, and I can’t touch it. \$\endgroup\$ – Ryan Mar 26 '16 at 21:42
  • \$\begingroup\$ @Ryan Oh okay. So that's a GUI builder generated code. Is that entire class generated by it? I didn't realize that from the code itself. Still, rename the variables to be camelCase; NetBeans won't require that. \$\endgroup\$ – Justin Mar 26 '16 at 21:43
1
\$\begingroup\$

Assumptions in PrimeCalculator

Your PrimeCalculator looping assumes that it will be checking for a prime number first in order to load its cache of prime factors correctly. If I were to run it first with a non-prime:

PrimeCalculator.isPrime(4);

It will loop against an empty ArrayList, and happily declare that 4 is a prime number since isPrime == true.

Interfaces over implementations in declaration

public static ArrayList<Integer> primeNumberCache = new ArrayList<>();

This should be declared as a List, so that users of primeNumberCache simply know that it adheres to a List, instead of knowing its underlying implementation.

Names

You can do with a rename of the method argument primeToCheck, since you aren't checking a prime number, but checking a number to see if it is prime.

sqrtOfCurrentIteration is incorrectly labelled too, as you are taking the square root of primeToCheck every time.

Looping conditions

Even in your current code, you can use the standard for loop so that you can introduce loop terminal conditions, instead of having to manually break within the loop:

boolean isPrime = true;
double sqrt = Math.sqrt(number); // renamed primeToCheck to number
for (int i = 0; i < primeNumberCache.size()
        && primeNumberCache.get(i) < sqrt
        && (isPrime = (number % primeNumberCache.get(i) != 0)); i++) {
    // empty
}
if (isPrime) {
    primeNumberCache.add(number);
    return true;
} else {
    return false;
}

Boolean comparison

if (isPrime == true) can be rewritten as if (isPrime).

\$\endgroup\$
  • \$\begingroup\$ Thanks. I actually believe that I meant to make primeCache private instead of public. \$\endgroup\$ – Ryan Mar 27 '16 at 14:58

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.