3
\$\begingroup\$

I was asked to make a Java application with main class q2.

How can I make it better and more efficient?

import java.awt.*;
import java.awt.event.*;
import javax.swing.*;
import java.util.*;

class Sort extends q2
{
    public static double SWITCH;
}
class q2 extends JFrame
{
    public static void main(String[] args)
    {
        q2 window = new q2();
        Container cont = window.getContentPane();
        window.setDefaultCloseOperation(EXIT_ON_CLOSE);
        Dimension dim = Toolkit.getDefaultToolkit().getScreenSize();
        window.setVisible(true);
        window.setSize(dim);
        JPanel LeftPanel = new JPanel();
        JPanel RightPanel = new JPanel();
        JPanel MiddlePanel = new JPanel();
        JPanel Middle1Panel =new JPanel();
        JPanel Middle2Panel = new JPanel();
        JPanel Middle3Panel = new JPanel();
        MiddlePanel.add(Middle1Panel);
        MiddlePanel.add(Middle2Panel);
        MiddlePanel.add(Middle3Panel);
        cont.setLayout(new GridLayout(1,3));
        cont.add(LeftPanel);
        cont.add(MiddlePanel);
        cont.add(RightPanel);
        LeftPanel.setBackground(Color.red);
        RightPanel.setBackground(Color.black);

        JRadioButton BubbleButton = new JRadioButton("BubbleSort");
        JRadioButton SelectionButton = new JRadioButton("SelectionSort");
        JRadioButton InsertionButton = new JRadioButton("InsertionSort");
        RightPanel.add(BubbleButton);
        RightPanel.add(SelectionButton);
        RightPanel.add(InsertionButton);
        ButtonGroup Group1 = new ButtonGroup();
        Group1.add(InsertionButton);
        Group1.add(SelectionButton);
        Group1.add(BubbleButton);

        BubbleButton.addItemListener(new ItemListener() {
            public void itemStateChanged(ItemEvent e)
            {
                Sort.SWITCH = 1;
            }
        });
        SelectionButton.addItemListener(new ItemListener() {
            public void itemStateChanged(ItemEvent e)
            {
                Sort.SWITCH = 2;
            }
        });
        InsertionButton.addItemListener(new ItemListener() {
            public void itemStateChanged(ItemEvent e)
            {
                Sort.SWITCH = 3;
            }
        });

        LeftPanel.setLayout(new GridLayout(2,1));
        JPanel VariablePanel = new JPanel();
        JPanel ResultPanel = new JPanel();
        LeftPanel.add(VariablePanel);
        LeftPanel.add(ResultPanel);
        VariablePanel.setLayout(new GridLayout(10,1));
        final JTextField [] VarField = new JTextField[20];
        for(int q=0;q<10;q++)
        {
            VarField[q] = new JTextField();
            VariablePanel.add(VarField[q]);
        }

        ResultPanel.setBackground(Color.black);
        ResultPanel.setLayout(new GridLayout(10,10));
        JButton SortButton = new JButton("Sort");
        ResultPanel.add(SortButton);

        Middle1Panel.setLayout(new GridLayout(4,2));

        final JTextField Output = new JTextField(50);
        Output.setText("OutputMinute");
        ResultPanel.add(Output);

        JLabel Min = new JLabel("Min");
        Middle1Panel.add(Min);
        final JTextField OutputMin = new JTextField(50);
        Output.setText("OutputSecond");
        Middle1Panel.add(OutputMin);
        JLabel Sec = new JLabel("Sec");
        Middle1Panel.add(Sec);
        final JTextField OutputSec = new JTextField(50);
        Output.setText("Output");
        Middle1Panel.add(OutputSec);
        JLabel Mill = new JLabel("Millisec");
        Middle1Panel.add(Mill);
        final JTextField OutputMill = new JTextField(50);
        Output.setText("Output");
        Middle1Panel.add(OutputMill);
        JButton Randomise = new JButton("Randomise");
        Middle1Panel.add(Randomise);

        final Random randomise = new Random();
        Randomise.addActionListener(new ActionListener()  {
            public void actionPerformed(ActionEvent a)
            {
                for(int q=0;q<=9;q++)
                {
                    VarField[q].setText(Integer.toString(randomise.nextInt()));
                }
            }
        });

        SortButton.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent a)
            {
                if (Sort.SWITCH == 1)
                {
                    Calendar bubblestart = Calendar.getInstance();
                    int w,mindiff,secdiff,millsecdiff,q;
                    int[] i =new int[10];
                    for(q=0;q<10;q++)
                    {
                        i[q]=Integer.parseInt(VarField[q].getText());
                    }
                    while (q != 0)
                    {
                        q=0;
                        for(int r=0;r<=8;r++)
                        {
                            if (i[r]>i[r+1])
                            {
                                q=1;
                                w=i[r];
                                i[r]=i[r+1];
                                i[r+1]=w;
                            }
                        }
                    }

                    Calendar bubbleend = Calendar.getInstance();
                    Output.setText(Integer.toString(i[0])+" "+Integer.toString(i[1])+" "+Integer.toString(i[2])+" "+Integer.toString(i[3])+" "+Integer.toString(i[4])+" "+Integer.toString(i[5])+" "+Integer.toString(i[6])+" "+Integer.toString(i[7])+" "+Integer.toString(i[8])+" "+Integer.toString(i[9]));
                    mindiff=bubbleend.get(Calendar.MINUTE)-bubblestart.get(Calendar.MINUTE);
                    secdiff=bubbleend.get(Calendar.SECOND)-bubblestart.get(Calendar.SECOND);
                    millsecdiff=bubbleend.get(Calendar.MILLISECOND)-bubblestart.get(Calendar.MILLISECOND);
                    OutputMin.setText(Integer.toString(mindiff));
                    OutputSec.setText(Integer.toString(secdiff));
                    OutputMill.setText(Integer.toString(millsecdiff));
                }
                else if (Sort.SWITCH == 2)
                {
                    Calendar selectstart = Calendar.getInstance();
                    int w,serial=0,mindiff,secdiff,millsecdiff,q;
                    int[] i =new int[10];
                    for(q=0;q<10;q++)
                    {
                        i[q]=Integer.parseInt(VarField[q].getText());
                    }
                    for (q=0;q<9;q++)
                    {
                        w=i[q];
                        for(int r=q;r<=9;r++)
                        {
                            if(i[r]<w)
                            {
                                w=i[r];
                                serial=r;
                            }
                        }
                        if(i[q]!=w)
                        {
                            w=i[q];
                            i[q]=i[serial];
                            i[serial]=w;
                        }
                    }
                    Calendar selectend = Calendar.getInstance();
                    Output.setText(Integer.toString(i[0])+" "+Integer.toString(i[1])+" "+Integer.toString(i[2])+" "+Integer.toString(i[3])+" "+Integer.toString(i[4])+" "+Integer.toString(i[5])+" "+Integer.toString(i[6])+" "+Integer.toString(i[7])+" "+Integer.toString(i[8])+" "+Integer.toString(i[9]));
                    mindiff=selectend.get(Calendar.MINUTE)-selectstart.get(Calendar.MINUTE);
                    secdiff=selectend.get(Calendar.SECOND)-selectstart.get(Calendar.SECOND);
                    millsecdiff=selectend.get(Calendar.MILLISECOND)-selectstart.get(Calendar.MILLISECOND);
                    OutputMin.setText(Integer.toString(mindiff));
                    OutputSec.setText(Integer.toString(secdiff));
                    OutputMill.setText(Integer.toString(millsecdiff));
                }
                else if (Sort.SWITCH == 3)
                {
                    Calendar insertstart = Calendar.getInstance();
                    int q,mindiff,secdiff,millsecdiff,loop1,loop2,temp1,temp2=0;
                    int[] i =new int[10];
                    for(q=0;q<10;q++)
                    {
                        i[q]=Integer.parseInt(VarField[q].getText());
                    }
                    for(loop1=1;loop1<=9;loop1++)
                    {
                        int in, out;
                        for(out=1; out<10; out++)
                        {
                            int temp = i[out];
                            in = out;
                            while(in>0 && i[in-1] >= temp)
                            {
                                i[in] = i[in-1];
                                --in;
                            }
                            i[in] = temp;
                        }
                    }
                    Calendar insertend = Calendar.getInstance();
                    Output.setText(Integer.toString(i[0])+" "+Integer.toString(i[1])+" "+Integer.toString(i[2])+" "+Integer.toString(i[3])+" "+Integer.toString(i[4])+" "+Integer.toString(i[5])+" "+Integer.toString(i[6])+" "+Integer.toString(i[7])+"      "+Integer.toString(i[8])+" "+Integer.toString(i[9]));
                    mindiff=insertend.get(Calendar.MINUTE)-insertstart.get(Calendar.MINUTE);
                    secdiff=insertend.get(Calendar.SECOND)-insertstart.get(Calendar.SECOND);
                    millsecdiff=insertend.get(Calendar.MILLISECOND)-insertstart.get(Calendar.MILLISECOND);
                    OutputMin.setText(Integer.toString(mindiff));
                    OutputSec.setText(Integer.toString(secdiff));
                    OutputMill.setText(Integer.toString(millsecdiff));
                }
                else
                {
                    Output.setText("Select a Sorting method");
                }
            }
        });
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ please learn java naming conventions and stick to them \$\endgroup\$
    – kleopatra
    Commented Jan 18, 2012 at 16:26

2 Answers 2

8
\$\begingroup\$

Unless you absolutely must keep everything in main, this should probably be split up into a number of separate functions. At the very least, I'd think of:

  1. One function for the UI.
  2. One function for each sort.
  3. One function to randomize the data.

If it were up to me, I think I'd split it out a bit more than this as well, with one function to create the UI, and separate one to update the UI with results. In reality, a lot of creating the UI is also pretty much identical code repeated three times, so you probably want to split that into a couple of pieces as well, with one to handle only the repeated part (that you can call three times) and another that handles the parts that aren't repeated per-button.

If you want it to be nicer from an OO viewpoint, you could go a bit further than that, and define a sort interface, and then have selectionSort, insertionSort and bubbleSort each implement that interface. Right now you also have essentially identical timing code in triplicate (once for each sort); you'd probably be better off isolating that in one place as well, and applying it to each sort as you run it.

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

Revision

Slightly longer but a lot more readable/extendable.

SortQ2Program.java

import javax.swing.JFrame;
import javax.swing.SwingUtilities;

public class SortQ2Program implements Runnable {

    @Override
    public void run() {
        JFrame frame = new JFrame();
        frame.setContentPane(new SortQ2Panel());
        frame.pack();
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setVisible(true);
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new SortQ2Program());
    }

}

SortQ2Panel.java

import java.awt.Color;
import java.awt.Dimension;
import java.awt.GridLayout;
import java.awt.Toolkit;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.HashMap;
import java.util.Map;
import java.util.Random;
import java.util.concurrent.TimeUnit;

import javax.swing.ButtonGroup;
import javax.swing.JButton;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JRadioButton;
import javax.swing.JTextArea;
import javax.swing.JTextField;

public class SortQ2Panel extends JPanel {

    private static interface SortingAlgorithm {

        public void sort(int[] values);

    }

    private static abstract class AbstractSortingAlgorithm
            implements SortingAlgorithm {

        protected void swap(int[] values, int aIndex, int bIndex) {
            int temp = values[aIndex];
            values[aIndex] = values[bIndex];
            values[bIndex] = temp;
        }

    }

    private static class BubbleSort extends AbstractSortingAlgorithm {

        @Override
        public void sort(int[] values) {
            boolean isSorted = false;

            while (!isSorted) {
                isSorted = true;

                for (int i = 0; i < N_VALUE_FIELDS - 1; ++i) {
                    if (values[i] > values[i + 1]) {
                        isSorted = false;
                        swap(values, i, i + 1);
                    }
                }
            }
        }

    }

    private static class SelectionSort extends AbstractSortingAlgorithm {

        @Override
        public void sort(int[] values) {
            for (int index = 0; index < N_VALUE_FIELDS; ++index) {
                int minIndex = findMinIndex(values, index);

                if (minIndex != index) {
                    swap(values, index, minIndex);
                }
            }
        }

        private int findMinIndex(int[] values, int startIndex) {
            int minIndex = startIndex;

            for (int index = startIndex; index < N_VALUE_FIELDS; ++index) {
                if (values[index] < minIndex) {
                    minIndex = index;
                }
            }

            return minIndex;
        }

    }

    private static class InsertionSort extends AbstractSortingAlgorithm {

        @Override
        public void sort(int[] values) {
            for (int index = 0; index < N_VALUE_FIELDS; ++index) {
                int insertionValue = values[index];
                int insertionIndex = findInsertionIndex(values, index,
                        insertionValue);
                values[insertionIndex] = insertionValue;
            }
        }

        private int findInsertionIndex(int[] values, int startIndex,
                int value) {
            int result;

            for (result = startIndex;
                    result > 0 && values[result - 1] > value; --result) {
                values[result] = values[result - 1];
            }

            return result;
        }

    }

    private static final long serialVersionUID = 1L;

    private static final int ROWS = 1;
    private static final int COLS = 3;

    private static final int N_VALUE_FIELDS = 10;

    private static final int OUTPUT_ROWS = 2;
    private static final int OUTPUT_COLS = 50;

    private static final int LEFT_PANEL_ROWS = 2;

    private static final int MIDDLE_PANEL_ROWS = 4;

    private static final int MIDDLE_PANEL_COLS = 2;

    private static final int RESULT_PANEL_ROWS = 2;

    @SuppressWarnings("serial")
    private static final Map<String, SortingAlgorithm> SORTING_MAP =
            new HashMap<String, SortingAlgorithm>() {{
        put("BubbleSort", new BubbleSort());
        put("SelectionSort", new SelectionSort());
        put("InsertionSort", new InsertionSort());
    }};

    private static final Random RANDOM_NUMBER_GENERATOR = new Random();

    private ButtonGroup mSortGroup = new ButtonGroup();

    private JTextField[] mNumFields = new JTextField[N_VALUE_FIELDS];

    private JTextArea mOutputField = new JTextArea("Output", OUTPUT_ROWS, OUTPUT_COLS);

    private JTextField mOutputMinField = new JTextField("OutputMin",
            OUTPUT_COLS);
    private JTextField mOutputSecField = new JTextField("OutputSec",
            OUTPUT_COLS);
    private JTextField mOutputMillisecField = new JTextField("OutputMil",
            OUTPUT_COLS);

    public SortQ2Panel() {
        super(new GridLayout(ROWS, COLS));
        initComponents();
    }

    @Override
    public Dimension getPreferredSize() {
        return Toolkit.getDefaultToolkit().getScreenSize();
    }

    private void initComponents() {
        mOutputField.setLineWrap(true);
        initLeftPanel();
        initMiddlePanel();
        initRightPanel();
    }

    private void initLeftPanel() {
        JPanel leftPanel = new JPanel(new GridLayout(LEFT_PANEL_ROWS, 1));
        leftPanel.setBackground(Color.RED);
        leftPanel.add(initNumPanel());
        leftPanel.add(initResultPanel());
        add(leftPanel);
    }

    private void initMiddlePanel() {
        JPanel middlePanel = new JPanel(
                new GridLayout(MIDDLE_PANEL_ROWS, MIDDLE_PANEL_COLS));
        middlePanel.add(new JLabel("Min"));
        middlePanel.add(mOutputMinField);
        middlePanel.add(new JLabel("Sec"));
        middlePanel.add(mOutputSecField);
        middlePanel.add(new JLabel("Millisec"));
        middlePanel.add(mOutputMillisecField);

        JButton randomiseButton = new JButton("Randomise");
        randomiseButton.addActionListener(new ActionListener() {

            @Override
            public void actionPerformed(ActionEvent e) {
                for (JTextField numField : mNumFields) {
                    int randInt = RANDOM_NUMBER_GENERATOR.nextInt();
                    numField.setText(String.valueOf(randInt));
                }
            }

        });

        middlePanel.add(randomiseButton);
        add(middlePanel);
    }

    private void initRightPanel() {
        JPanel radioPanel = new JPanel();
        radioPanel.setBackground(Color.BLACK);

        for (String buttonName : SORTING_MAP.keySet()) {
            JRadioButton button = new JRadioButton(buttonName);
            button.setActionCommand(buttonName);
            button.setForeground(Color.WHITE);
            mSortGroup.add(button);
            radioPanel.add(button);
        }

        add(radioPanel);
    }

    private JPanel initNumPanel() {
        JPanel numPanel = new JPanel(new GridLayout(N_VALUE_FIELDS, 1));

        for (int count = 0; count < N_VALUE_FIELDS; ++count) {
            mNumFields[count] = new JTextField();
            mNumFields[count].setEditable(false);
            numPanel.add(mNumFields[count]);
        }

        return numPanel;
    }

    private JPanel initResultPanel() {
        JPanel resultPanel = new JPanel(new GridLayout(RESULT_PANEL_ROWS, 1));
        resultPanel.setBackground(Color.BLACK);
        resultPanel.add(initSortButton());
        resultPanel.add(mOutputField);
        return resultPanel;
    }

    private JButton initSortButton() {
        JButton sortButton = new JButton("Sort");

        sortButton.addActionListener(new ActionListener() {

            @Override
            public void actionPerformed(ActionEvent e) {
                long start = System.currentTimeMillis();
                int[] values = parseValues();
                String sortType =
                        mSortGroup.getSelection().getActionCommand();
                SORTING_MAP.get(sortType).sort(values);
                long diff = System.currentTimeMillis() - start;
                outputValues(values);
                outputTime(diff);
            }

        });

        return sortButton;
    }

    private int[] parseValues() {
        int[] values = new int[N_VALUE_FIELDS];

        for (int index = 0; index < N_VALUE_FIELDS; ++index) {
            values[index] = Integer.parseInt(mNumFields[index].getText());
        }

        return values;
    }

    private void outputValues(int[] values) {
        String output = "";

        for (int value : values) {
            output += String.valueOf(value) + " ";
        }

        mOutputField.setText(output);
    }

    private void outputTime(long time) {
        long min = TimeUnit.MILLISECONDS.toMinutes(time);
        long sec = TimeUnit.MILLISECONDS.toSeconds(time) -
                TimeUnit.MINUTES.toSeconds(min);
        long millisec = time - TimeUnit.SECONDS.toMillis(sec);

        mOutputMinField.setText(String.valueOf(min));
        mOutputSecField.setText(String.valueOf(sec));
        mOutputMillisecField.setText(String.valueOf(millisec));
    }

}

Design Notes

  • setVisible should be the last thing called in your program. Before I moved it to the end, your program just showed up as a large blank.
  • Class names for java should be in UpperCamelCase.
  • Variable names for java should be lowerCamelCase.
  • You don't need to subclass your main class just to add a constant that you use in the main class anyway. Put the constant in the main class.
  • Don't use equals comparison with doubles. I think what you were going for with the SWITCH variable is an enum
  • Don't use wildcard imports like javax.swing.*. It pollutes the namespace. Import the individual classes instead.
  • Don't subclass JFrame. Instead subclass JPanel and make a separate class that holds the main method. Initialize a plain old JFrame there and set the content pane to your custom panel.
  • All GUI code should be inside SwingUtilities.invokeLater(Runnable). That puts it on the AWT event dispatching thread.

    From Sun's Threads and Swing article:

    Once a Swing component has been realized, all code that might affect or depend on the state of that component should be executed in the event-dispatching thread.

  • All methods should be short, including main. I hate making rules like 15 lines or less, but basically treat your code like a baby: if it smells change it.
  • SLVNAHTR (Single Letter Variable Names Are Hard To Read)
  • Variables should have one purpose and one purpose only.
  • private static final int NUMBER_OF_WAYS_MAGIC_NUMBERS_SUCK = 800;
    private static final String CONSTANTS = "better";
  • DRY code is good code.
  • I used the Strategy design pattern to simplify you complicated if statement. Now you can easily add a new sorting algorithm if you'd like. Just extend AbstractSortingAlgorithm and add the new algorithm to SORTING_MAP.
  • System.currentTimeMillis or System.nanoTime is usually used for timing things.
  • Get rid of unused variables. If you're trying to change the layout of the content pane, use the appropriate layout managers
  • My refactoring is purposefully incomplete. It's in desperate need of comments, some of the variable names could be better, the program is slightly useless, and the general look of the GUI is kinda ugly. Tis up to you to fix that.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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