0
\$\begingroup\$

I have written a GUI map application using the JSwing design interface. After I finished the coding phase and started to refactor, I found that there are a lot of repeated components in the code.

This is mainly caused by 20 toggle buttons and their action listeners. Since each button has its own property, it's pretty hard to refactor them to another class and they are also a part of GUI components so this makes the refactoring even harder...

So my question is, how can I manage this "Frankenstein", reduce its complexity and make it easy to maintain? Any advice on programming practices that I could apply to this code are also appreciated.

So here it goes:

package mainWindow.views;

import java.awt.EventQueue;
import java.awt.Graphics;
import java.awt.Graphics2D;
import javax.swing.JFrame;
import javax.swing.JOptionPane;
import javax.swing.JPanel;
import javax.swing.border.EmptyBorder;
//...

public class mainWindowGUI extends JFrame {

    /**
     * 
     */
    private static final long serialVersionUID = -18439880310575154L;

    private JPanel mainMenu;
    private JLabel lblSearchMethod;

    private JToggleButton tglbtnBfs;
    private JToggleButton tglbtnDfs;
    private JToggleButton tglbtnIds;

    private JButton btnReset;

    private JToggleButton loc_oradea;
    private JToggleButton loc_zerind;
    private JToggleButton loc_arad;
    //...

    private ActionListener locReceiver0;
    private ActionListener locReceiver1;
    //...

and other initial values

This is the main methods:

public static void main(String[] args) {
    EventQueue.invokeLater(new Runnable() {
        public void run() {
            try {
                mainWindowGUI frame = new mainWindowGUI();
                frame.setVisible(true);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    });
}

Outside main method: constructors, actionListeners, GUI generation methods, all the toggle buttons and methods for control flow...

public mainWindowGUI() {
    createEvents(); 
    layouts();
    locationBtns();
    buttons();
    labels();
    eventFlag = 0;
    startNode = 0; 
    graph.print();
}

public void eventFork(int eventFlag, int startNode){
    if(eventFlag == 1) {
        graph.bfs(startNode);
        graph.bfsCost();
        graph.bfsPath();
        System.out.println("\n---------------- Searh Terminated ----------------");
    }
    else if(eventFlag == 2){
        graph.dfs(startNode);
        graph.dfsCost();
        graph.dfsPath();
        System.out.println("\n---------------- Searh Terminated ----------------");
    }
    else if(eventFlag == 3){
        graph.ids(startNode);
        graph.idsCost();
        System.out.println("\n---------------- Searh Terminated ----------------");
    }
    else{
        JOptionPane.showMessageDialog(null, "No seraching method has been selected, please reset and select a searching method...");
    }
}

ActionListener methods: 

private void createEvents() {
        resetListener = new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent actionEvent) {
            AbstractButton abstractButton = (AbstractButton) actionEvent.getSource();
            boolean selected = abstractButton.getModel().isSelected();
            tglbtnBfs.setSelected(selected);            
            tglbtnDfs.setSelected(selected);
            tglbtnIds.setSelected(selected);

            tglbtnIds.setSelected(selected);
            loc_oradea.setSelected(selected);
            loc_zerind.setSelected(selected);
            loc_arad.setSelected(selected);
            loc_timisoara.setSelected(selected);
            loc_lugoj.setSelected(selected);
            loc_mehadia.setSelected(selected);
            loc_drobeta.setSelected(selected);
            loc_craiova.setSelected(selected);
            loc_giurgiu.setSelected(selected);
            loc_bucharest.setSelected(selected);
            loc_urziceni.setSelected(selected);
            loc_hirsova.setSelected(selected);
            loc_eforie.setSelected(selected);
            loc_vaslui.setSelected(selected);
            loc_iasi.setSelected(selected);
            loc_neamt.setSelected(selected);
            loc_fagaras.setSelected(selected);
            loc_rimnicuVilcea.setSelected(selected);
            loc_sibiu.setSelected(selected);
            loc_pitesti.setSelected(selected);
            loc_random.setSelected(selected);
        }
    };

    dialogListener = new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent e) {
            JOptionPane.showMessageDialog(null, "All reset!");
        }

    };

    bfsBeginptn = new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent e) {
            //BFS function
            JOptionPane.showMessageDialog(null, "BFS search selected, Please select a starting point...");
            eventFlag = 1;
        }

    };

    dfsBeginptn = new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent e) {
            //BFS function
            JOptionPane.showMessageDialog(null, "DFS search selected, Please select a starting point...");
            eventFlag = 2;
        }

    };

    idsBeginptn = new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent e) {
            //BFS function
            JOptionPane.showMessageDialog(null, "IDS search selected, Please select a starting point...");
            eventFlag = 3;
        }

    };
//and 17 other action listeners...
}

These are the toggle buttons:

private void locationBtns(){
    loc_bucharest = new JToggleButton("");
    loc_bucharest.setForeground(Color.BLACK);
    loc_bucharest.setBounds(582, 374, 15, 15);
    mainMenu.add(loc_bucharest);
    loc_bucharest.addActionListener(locReceiver9);

    loc_oradea = new JToggleButton("");
    loc_oradea.setBounds(228, 42, 15, 15);
    mainMenu.add(loc_oradea);
    loc_oradea.addActionListener(locReceiver0);

    loc_zerind = new JToggleButton("");
    loc_zerind.setBounds(199, 100, 15, 15);
    mainMenu.add(loc_zerind);
    loc_zerind.addActionListener(locReceiver1);
    //and 17 other buttons...
}

This method is for the GUI layout:

private void layouts() {
    setTitle("HiveMap");
    setIconImage(Toolkit.getDefaultToolkit().getImage(mainWindowGUI.class.getResource("/mainWindow/assets/icon.jpg")));
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    setBounds(100, 100, 930, 590);
    mainMenu = new JPanel();
    mainMenu.setBorder(new EmptyBorder(5, 5, 5, 5));
    setContentPane(mainMenu);
    mainMenu.setLayout(null);
 }

This is for all the labels:

private void labels() {
    super.paint(g);

    Point2D.Float oradea = new Point2D.Float(245, 88);
    Point2D.Float zerind = new Point2D.Float(216, 146);
    Point2D.Float arad = new Point2D.Float(193, 202);
    Point2D.Float timisoara = new Point2D.Float(193, 310);
    Point2D.Float Lugoj = new Point2D.Float(285, 352);
    Point2D.Float mehadia = new Point2D.Float(289, 403);
    Point2D.Float drobeta = new Point2D.Float(285, 454);
    Point2D.Float craiova = new Point2D.Float(403, 469);
    Point2D.Float giurgiu = new Point2D.Float(565, 493);
    Point2D.Float bucharest = new Point2D.Float(599, 420);
    Point2D.Float urziceni = new Point2D.Float(663,390);
    Point2D.Float hirsova = new Point2D.Float(777, 403);
    Point2D.Float eforie = new Point2D.Float(815, 469);
    Point2D.Float vaslui = new Point2D.Float(747, 268);
    Point2D.Float iasi = new Point2D.Float(697, 177);
    Point2D.Float neamt = new Point2D.Float(613, 146);
    Point2D.Float fagaras = new Point2D.Float(479, 257);
    Point2D.Float rimnicuVilcea = new Point2D.Float(382, 310);
    Point2D.Float sibiu = new Point2D.Float(350, 245);
    Point2D.Float pitesti = new Point2D.Float(492, 368);

    Graphics2D g2 = (Graphics2D) g;

    Line2D lin1 = new Line2D.Float(oradea, zerind);
    Line2D lin2 = new Line2D.Float(zerind, arad);
    Line2D lin3 = new Line2D.Float(arad, timisoara);
    Line2D lin4 = new Line2D.Float(timisoara, Lugoj);
    Line2D lin5 = new Line2D.Float(Lugoj, mehadia);
    Line2D lin6 = new Line2D.Float(mehadia, drobeta);
    Line2D lin7 = new Line2D.Float(drobeta, craiova);
    Line2D lin8 = new Line2D.Float(craiova, pitesti);
    Line2D lin9 = new Line2D.Float(pitesti, bucharest);
    Line2D lin10 = new Line2D.Float(bucharest, giurgiu);
    Line2D lin11 = new Line2D.Float(bucharest, urziceni);
    Line2D lin12 = new Line2D.Float(urziceni, hirsova);
    Line2D lin13 = new Line2D.Float(hirsova, eforie);
    Line2D lin14 = new Line2D.Float(urziceni, vaslui);
    Line2D lin15 = new Line2D.Float(vaslui, iasi);
    Line2D lin16 = new Line2D.Float(iasi, neamt);
    Line2D lin17 = new Line2D.Float(bucharest, fagaras);
    Line2D lin18 = new Line2D.Float(fagaras, sibiu);
    Line2D lin19 = new Line2D.Float(sibiu, rimnicuVilcea);
    Line2D lin20 = new Line2D.Float(rimnicuVilcea, craiova);
    Line2D lin21 = new Line2D.Float(rimnicuVilcea, pitesti);
    Line2D lin22 = new Line2D.Float(sibiu, oradea);
    Line2D lin23 = new Line2D.Float(sibiu, arad);

    g2.draw(lin1);
    g2.draw(lin2);
    g2.draw(lin3);
    g2.draw(lin4);
    g2.draw(lin5);
    g2.draw(lin6);
    g2.draw(lin7);
    g2.draw(lin8);
    g2.draw(lin9);
    g2.draw(lin10);
    g2.draw(lin11);
    g2.draw(lin12);
    g2.draw(lin13);
    g2.draw(lin14);
    g2.draw(lin15);
    g2.draw(lin16);
    g2.draw(lin17);
    g2.draw(lin18);
    g2.draw(lin19);
    g2.draw(lin20);
    g2.draw(lin21);
    g2.draw(lin22);
    g2.draw(lin23);
}//end of paint()
}//end of class
|improve this question
\$\endgroup\$
  • 3
    \$\begingroup\$ You should try to come up with a different title that describes what your code does instead of what you want out of a review. The current title is applicable to too many question on this site. \$\endgroup\$ – Graipher Jul 17 '17 at 9:34
  • \$\begingroup\$ I have changed the title that I think would be more fitting for my question, thanks for your advice. \$\endgroup\$ – John Jul 19 '17 at 0:08
  • \$\begingroup\$ That title still does not reflect what your code does, except that it is some GUI in Java. \$\endgroup\$ – Graipher Jul 19 '17 at 16:53
2
\$\begingroup\$

Arrays and loops are your friend.

g2.draw(lin1);
g2.draw(lin2);
g2.draw(lin3);
g2.draw(lin4);
g2.draw(lin5);
g2.draw(lin6);
g2.draw(lin7);
g2.draw(lin8);
g2.draw(lin9);
g2.draw(lin10);
g2.draw(lin11);
g2.draw(lin12);
g2.draw(lin13);
g2.draw(lin14);
g2.draw(lin15);
g2.draw(lin16);
g2.draw(lin17);
g2.draw(lin18);
g2.draw(lin19);
g2.draw(lin20);
g2.draw(lin21);
g2.draw(lin22);
g2.draw(lin23);

If you held all these Line2D as an array named lines could refactor the above as:

for (Line2D line : lines) {
    g2.draw(line);
}

Similarly, if you declare your Point2DFloat as an array you could simply match the right points using index e.g.

pointfloats[0] = new Point2D.Float(245, 88);
        // etc...

for (int i = 0, j = 0; i < lines.length; i++, j += 2) {
    lines[i] = new Line2D.Float(pointfloats[j], pointfloats[j + 1]);
}

Do note that we're still declaring the points line by line. This is undesirable and you have 'magic numbers' all over the place. A better approach would be to externalize this data, and read it all into a data array so you can just have 3 arrays and loops to connect them. This also has the added benefit of not needing to compile for a data change.

This really goes for most of this code, as all that needs to be done is to hold the objects in an array e.g. your ActionListener setSelected procedure should just be 1 loop.

|improve this answer
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your input, I will look into it. \$\endgroup\$ – John Jul 18 '17 at 20:32

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.