Results 1 to 7 of 7
Like Tree1Likes
  • 1 Post By Fubarable

Thread: Messy code clean up

  1. #1
    Zyril is offline Senior Member
    Join Date
    Oct 2011
    Location
    Sweden
    Posts
    124
    Rep Power
    0

    Default Messy code clean up

    Hey again people, I'm not sure what the general view is on people who start thread after thread, but I feel that I learn so much by just reading threads and asking questions that it is worth it. At least I am trying to solve my problems single handedly before asking anyone else.

    I've read into different layoutmanagers in Swing, and for my GUI I've used GroupLayout. Though, I am having some trouble understanding my own autogenerated code, and what it all is. Also, I feel like the code is very messy, and I'm not sure how to structure it. Are there any good guidelines on how to structure the GUI code, like doing it in different layers and separating things? Right now I'm not sure what I must have to make it work, or if it is something I can remove entirely. Any tips on this subject is very welcome!


    I'm trying to add a JTable with column namnes, but they will not be displayed. I only get a blank JTable frame.



    This is how my GUI looks like right now:

    Messy code clean up-yc4ut.png

    I want the table to be displayed with column names like this:

    Messy code clean up-g4kwv.png

    The code is the same in both projects, though I am using a BorderLayout in the working JTable, and GroupLayout in the one that doesn't work.

    I'm sorry for the huge code post below. I'm not sure why part of the code is blue'd out, because it isn't in Eclipse and it's running fine without errors or warnings.

    Java Code:
    
    package winekeeper.view;
    
    
    import java.awt.BorderLayout;
    import java.awt.EventQueue;
    
    import javax.swing.JFrame;
    import javax.swing.JPanel;
    import javax.swing.border.EmptyBorder;
    import javax.swing.table.DefaultTableModel;
    
    import javax.swing.JButton;
    import javax.swing.GroupLayout;
    import javax.swing.GroupLayout.Alignment;
    import javax.swing.JScrollPane;
    import javax.swing.UIManager;
    import javax.swing.UIManager.LookAndFeelInfo;
    
    
    import javax.swing.LayoutStyle.ComponentPlacement;
    
    import winekeeper.controller.Controller;
    import java.awt.event.ActionListener;
    import java.awt.event.ActionEvent;
    
    import java.io.IOException;
    
    import javax.swing.JMenu;
    import javax.swing.JMenuBar;
    import javax.swing.JMenuItem;
    import javax.swing.JTextField;
    import javax.swing.JLabel;
    import javax.swing.JTable;
    
    public class WineKeeperGUI extends JFrame {
    
    	/**
    	 * 
    	 */
    	private static final long serialVersionUID = 1L;
    	private JPanel contentPane;
    	private JTextField txtArtikelnummer;
    	private int artnr;
    	private Controller controller;
    	
    	private final JLabel lblNamn = new JLabel("Namn:");
    	private final JLabel lblLand = new JLabel("Land:");
    	private JLabel lblVintage = new JLabel("\u00C5rg\u00E5ng:");
    	private JTextField txtAntal;
    	private JLabel lblPris = new JLabel("Pris:");
    	private JLabel lblLagring = new JLabel("Lagring:");
    	private JButton btnNewButton = new JButton("L\u00E4gg till");
    	private final JLabel lblDisplayName = new JLabel("");
    	private final JLabel lblDisplayVintage = new JLabel("");
    	private final JLabel lblDisplayCountry = new JLabel("");
    	private final JLabel lblDisplayPrice = new JLabel("");
    	private final JLabel lblAntal = new JLabel("Antal:");
    	private JLabel lblDisplayAntal = new JLabel("");
    	
    	//Menu items
    	private static JMenuBar menuBar;
    	private JMenu fileMenu;
    	private JMenuItem openMenu;
    	private JMenuItem exitMenu;
    
    
    	/**
    	 * Launch the application.
    	 */
    	public static void main(String[] args) {
    		EventQueue.invokeLater(new Runnable() {
    			
    			public void run() {
    				try {
    				    for (LookAndFeelInfo info : UIManager.getInstalledLookAndFeels()) {
    				        if ("Nimbus".equals(info.getName())) {
    				            UIManager.setLookAndFeel(info.getClassName());
    				            break;
    				        }
    				    }
    				} catch (Exception e) {
    				    // If Nimbus is not available, you can set the GUI to another look and feel.
    				}
    				try {
    					WineKeeperGUI frame = new WineKeeperGUI();
    					frame.setVisible(true);
    					frame.setJMenuBar(menuBar);
    				} catch (Exception e) {
    					e.printStackTrace();
    				}
    			}
    		});
    	}
    
    	
    	/**
    	 * Create the frame.
    	 */
    
    	public WineKeeperGUI() {
    		
    		controller = new Controller();
    		
    		setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    		setBounds(100, 100, 779, 413);
    		contentPane = new JPanel();
    		contentPane.setBorder(new EmptyBorder(5, 5, 5, 5));
    		contentPane.setLayout(new BorderLayout(0, 0));
    		setContentPane(contentPane);
    		
    
    
    		setTitle("Winekeeper");
    		// Create menu item
    		menuBar = new JMenuBar();
    		fileMenu = new JMenu("File");
    		openMenu = new JMenuItem("Open...");
    		exitMenu = new JMenuItem("Exit");
    		fileMenu.add(openMenu);
    		fileMenu.add(exitMenu);
    		menuBar.add(fileMenu);
    		
    		setJMenuBar(menuBar); //Add the menubar
    		setVisible(true);
    		
    		/*
    		 * Code for search button.
    		 */
    		JButton addSearchBtn = new JButton("S\u00F6k");
    		addSearchBtn.addActionListener(new ActionListener() {
    			
    			/* Action performed when "Sök"-button is pressed.
    			 * @see java.awt.event.ActionListener#actionPerformed(java.awt.event.ActionEvent)
    			 */
    			public void actionPerformed(ActionEvent arg0) {
    				artnr = Integer.parseInt(txtArtikelnummer.getText()); //This logic has to be moved outside GUI-class
    				try {
    					controller.searchWine(artnr);
    				} catch (IOException e) {
    					// TODO Auto-generated catch block
    					e.printStackTrace();
    				}
    				lblDisplayName.setText(controller.getCurrentName());
    				lblDisplayVintage.setText(Integer.toString(controller.getVintage()));
    				lblDisplayPrice.setText(controller.getPrice() + " kr");
    				lblDisplayCountry.setText(controller.getCountry());
    			}
    		});
    		//End of search button code
    		
    		
    		/* Action performed when "Lägg till"-button is pressed.
    		 * @see java.awt.event.ActionListener#actionPerformed(java.awt.event.ActionEvent)
    		 */
    		btnNewButton.addActionListener(new ActionListener() {
    			public void actionPerformed(ActionEvent arg0) {
    				controller.setAmount(Integer.parseInt(txtAntal.getText()));
    				controller.addWineToList();				
    				lblDisplayAntal.setText(controller.getAmount() + " st");
    			}
    		});
    		
    		/*
    		 * Table code
    		 */
            JTable tableWine = new JTable();
            DefaultTableModel model = new DefaultTableModel();
            tableWine.setModel(model);
            model.setColumnIdentifiers(new String[] {"Name", "Country", "Vintage", "Price"});
            JScrollPane tableContainer = new JScrollPane(tableWine);
    		//End of table code
    		
    		
    		/*
    		 *Textfield code 
    		 */
    		txtArtikelnummer = new JTextField();
    		txtArtikelnummer.setToolTipText("Artikelnummer");
    		txtArtikelnummer.setColumns(10);
    		txtAntal = new JTextField();
    		txtAntal.setToolTipText("Antal");
    		txtAntal.setColumns(10);
    		
    		//End of textfield code
    		
    
    		GroupLayout gl_contentPane = new GroupLayout(contentPane);
    		gl_contentPane.setHorizontalGroup(
    			gl_contentPane.createParallelGroup(Alignment.LEADING)
    				.addGroup(gl_contentPane.createSequentialGroup()
    					.addGroup(gl_contentPane.createParallelGroup(Alignment.LEADING)
    						.addGroup(gl_contentPane.createSequentialGroup()
    							.addComponent(txtArtikelnummer, GroupLayout.PREFERRED_SIZE, GroupLayout.DEFAULT_SIZE, GroupLayout.PREFERRED_SIZE)
    							.addPreferredGap(ComponentPlacement.RELATED)
    							.addComponent(addSearchBtn))
    						.addGroup(gl_contentPane.createSequentialGroup()
    							.addGroup(gl_contentPane.createParallelGroup(Alignment.LEADING)
    								.addComponent(btnNewButton)
    								.addGroup(gl_contentPane.createSequentialGroup()
    									.addContainerGap()
    									.addGroup(gl_contentPane.createParallelGroup(Alignment.TRAILING)
    										.addComponent(lblPris)
    										.addComponent(lblVintage)
    										.addComponent(lblLagring)
    										.addComponent(lblLand)
    										.addComponent(lblNamn)
    										.addComponent(lblAntal))))
    							.addPreferredGap(ComponentPlacement.RELATED)
    							.addGroup(gl_contentPane.createParallelGroup(Alignment.LEADING)
    								.addComponent(lblDisplayName)
    								.addComponent(txtAntal, GroupLayout.PREFERRED_SIZE, 40, GroupLayout.PREFERRED_SIZE)
    								.addComponent(lblDisplayVintage)
    								.addComponent(lblDisplayCountry)
    								.addComponent(lblDisplayPrice)
    								.addComponent(lblDisplayAntal))
    							.addPreferredGap(ComponentPlacement.RELATED, 210, Short.MAX_VALUE)
    							.addComponent(tableWine, GroupLayout.PREFERRED_SIZE, 412, GroupLayout.PREFERRED_SIZE)))
    					.addContainerGap())
    		);
    		
    		gl_contentPane.setVerticalGroup(
    			gl_contentPane.createParallelGroup(Alignment.LEADING)
    				.addGroup(gl_contentPane.createSequentialGroup()
    					.addGroup(gl_contentPane.createParallelGroup(Alignment.LEADING)
    						.addGroup(gl_contentPane.createSequentialGroup()
    							.addGroup(gl_contentPane.createParallelGroup(Alignment.BASELINE)
    								.addComponent(txtArtikelnummer, GroupLayout.PREFERRED_SIZE, GroupLayout.DEFAULT_SIZE, GroupLayout.PREFERRED_SIZE)
    								.addComponent(addSearchBtn))
    							.addPreferredGap(ComponentPlacement.RELATED)
    							.addGroup(gl_contentPane.createParallelGroup(Alignment.BASELINE)
    								.addComponent(btnNewButton)
    								.addComponent(txtAntal, GroupLayout.PREFERRED_SIZE, GroupLayout.DEFAULT_SIZE, GroupLayout.PREFERRED_SIZE))
    							.addPreferredGap(ComponentPlacement.RELATED)
    							.addGroup(gl_contentPane.createParallelGroup(Alignment.BASELINE)
    								.addComponent(lblNamn)
    								.addComponent(lblDisplayName))
    							.addPreferredGap(ComponentPlacement.RELATED)
    							.addGroup(gl_contentPane.createParallelGroup(Alignment.BASELINE)
    								.addComponent(lblLand)
    								.addComponent(lblDisplayCountry))
    							.addPreferredGap(ComponentPlacement.RELATED)
    							.addGroup(gl_contentPane.createParallelGroup(Alignment.BASELINE)
    								.addComponent(lblVintage)
    								.addComponent(lblDisplayVintage))
    							.addPreferredGap(ComponentPlacement.RELATED)
    							.addGroup(gl_contentPane.createParallelGroup(Alignment.BASELINE)
    								.addComponent(lblPris)
    								.addComponent(lblDisplayPrice))
    							.addPreferredGap(ComponentPlacement.RELATED)
    							.addComponent(lblLagring)
    							.addPreferredGap(ComponentPlacement.RELATED)
    							.addGroup(gl_contentPane.createParallelGroup(Alignment.BASELINE)
    								.addComponent(lblAntal)
    								.addComponent(lblDisplayAntal)))
    						.addGroup(gl_contentPane.createSequentialGroup()
    							.addGap(29)
    							.addComponent(tableWine, GroupLayout.PREFERRED_SIZE, 226, GroupLayout.PREFERRED_SIZE)))
    					.addContainerGap(110, Short.MAX_VALUE))
    		);
    		
    		contentPane.setLayout(gl_contentPane);
    		contentPane.add(tableContainer);
    	}
    }

    Any thoughts and tips?
    Thanks,
    Z!

  2. #2
    Fubarable's Avatar
    Fubarable is offline Moderator
    Join Date
    Jun 2008
    Posts
    19,316
    Blog Entries
    1
    Rep Power
    26

    Default Re: Messy code clean up

    Don't try to structure GroupLayout or edit the autogenerated code. Many here, myself included think you're much better off studying the layout managers and learning to hand -code your layouts. Remember that often you will want to nest JPanels, each using its own layout.
    Zyril likes this.

  3. #3
    DarrylBurke's Avatar
    DarrylBurke is offline Forum Police
    Join Date
    Sep 2008
    Location
    Madgaon, Goa, India
    Posts
    11,419
    Rep Power
    20

    Default Re: Messy code clean up

    Please go through the Forum Rules -- particularly the third paragraph.

    db
    If you're forever cleaning cobwebs, it's time to get rid of the spiders.

  4. #4
    Zyril is offline Senior Member
    Join Date
    Oct 2011
    Location
    Sweden
    Posts
    124
    Rep Power
    0

    Default Re: Messy code clean up

    Thanks for the reply Fubar!

    I will read about using different layouts depending on purpose, and then nest the JPanels the layouts are applied to.

    Right now, I'm sticking to the autogenerated GroupLayout code, since I just want to see this program working!

    I've added a JScrollPane to display the table as I want, but I can't populate the table correctly. I wrote a method to populate it but it didn't work. Now I've just added a single code line in a button actionlistener to see if I can populate it that way, but that also fails. Can you spot any obvious errors in my thought process?

    JTable code
    Java Code:
            JTable tableWine = new JTable();
            model = new DefaultTableModel();
            tableWine.setModel(new DefaultTableModel(
            	new Object[][] {
            	},
            	new String[] {
            		"Namn", "Land", "\u00C5rg\u00E5ng", "Pris", "Antal", "Lagring"
            	}
            ));
    		JScrollPane tableContainer = new JScrollPane(tableWine);
    Button ActionListener
    Java Code:
    		btnAddButton.addActionListener(new ActionListener() {
    			public void actionPerformed(ActionEvent arg0) {
    				controller.setAmount(Integer.parseInt(txtAntal.getText()));
    				controller.addWineToList();				
    				lblDisplayAntal.setText(controller.getAmount() + " st");
    				model.addRow(new String[] {"Test", "Test", "Test", "Test", "Test", "Test"});
    			}
    		});
    There are no exceptions thrown, nothing happens. I was wondering whether or not I will have to add rows before I can populate them, but isn't that exactly what I am doing? I mean, do I have to have empty rows before I can add content to it, or can I add rows and content at the same time?

    Thanks,
    Z!

    Thanks,

  5. #5
    Fubarable's Avatar
    Fubarable is offline Moderator
    Join Date
    Jun 2008
    Posts
    19,316
    Blog Entries
    1
    Rep Power
    26

    Default Re: Messy code clean up

    The DefaultTableModel object that model refers to is created:
    Java Code:
    model = new DefaultTableModel();
    ... but never added to the JTable. Instead you create a completely different anonymous model and place it into the JTable:
    Java Code:
    tableWine.setModel(new DefaultTableModel(
        new Object[][] {
        },
        new String[] {
            "Namn", "Land", "\u00C5rg\u00E5ng", "Pris", "Antal", "Lagring"
        }
    ));
    So it should come as no surprise that adding data to the model variable will have no effect on the JTable. Instead, don't create your anonymous model and instead do all your work with the model variable.

  6. #6
    Zyril is offline Senior Member
    Join Date
    Oct 2011
    Location
    Sweden
    Posts
    124
    Rep Power
    0

    Default Re: Messy code clean up

    Thank you Fubar, that worked out splendid!

    Darryl, I will keep that in mind!


    I have a follow up question, that is more close to OO-programming than Java in particular, but perhaps I could get some good thoughts from you guys.

    I've had a course in Object Oriented Design, and I've applied the MVC pattern to the program that is the subject of this thread. I've tried to read up on the MVC pattern and different use of other patterns, and there are a few questions that have surfaced.

    Logic in the View package
    From what I've learned, the view package should only contain GUI code, no logic whatsoever. My JTable and DefaultTableModel is in the GUI class in the View package, and I want to populate the table with information using a for-loop. Would this be considered bad OO programming, since I have the for-loop in the GUI class? Should I move the logic to the controller, populate the model and return it and apply it the to table?

    Using an interface to notify of changes in the ArrayList
    I have dived deeper into different OO design patterns, and I've looked into the Observer pattern, which some times seem to be called Subscription/Publisher pattern. The program I'm writing add wines to an ArrayList, and the ArrayList is being presented in the GUI. The ArrayList is part of the Model in the MVC pattern, and therefor no communication should exist between that and the GUI. Would it, from an OOD view, be a good idea to apply the Observer pattern and establish an interface that notifies the view of changes in the ArrayList, and by that way upholding the ideas and "rules" of the MVC pattern?


    ---

    On an off topic note, I just have to say how much I appreciate this forum! I've been on several others trying to "lurk" and read what other people ask, but nowhere else have I found the dedication of some of you experienced developers to help us newbies that are lost in the jungle of API's and misconceptions!

    This means so much for my learning progress, and I'm sure there are so many more people who started out just like me, lurking, that benefit alot from your answers!

    Thank you!
    Last edited by Zyril; 08-07-2012 at 10:19 PM.

  7. #7
    Fubarable's Avatar
    Fubarable is offline Moderator
    Join Date
    Jun 2008
    Posts
    19,316
    Blog Entries
    1
    Rep Power
    26

    Default Re: Messy code clean up

    I'm not a professional coder, so please take anything I say with a grain of salt, especially with regards to MVC design use in professional grade applications.

    My impression is that in some GUIs there may be different "granularities" of MVC or related patterns, that overall there's a large-scale MVC that may involve a database and other code for the logic portion, a GUI for display and a controller for interactions between the two, but that there also may be smaller scale MVC-like entities especially with regard to some Swing components. Regarding your specific question, your ArrayList could be held by a class that extends AbstractTableModel and be part of the model, using the notify methods of the model to notify the view of changes to the data. Alternatively, the you could implement another layer of indirection and have your ArrayList held in class that is completely UI-agnostic, but giving it the ability to be listened to for changes, such as by giving it a PropertyChangeSupport object as well as add/remove methods for PropertyChangeListeners, and then have your table model hold an instance of this class as its nucleus.

Similar Threads

  1. What to clean up in destroyApp?
    By 0circle0 in forum CLDC and MIDP
    Replies: 11
    Last Post: 07-08-2011, 12:13 AM
  2. Is there a way to clean up this recursion code?
    By SMHouston in forum New To Java
    Replies: 2
    Last Post: 09-30-2009, 02:30 AM
  3. [REQ] could anybody clean this method up for me?
    By harryblue in forum Advanced Java
    Replies: 5
    Last Post: 03-19-2009, 02:37 AM
  4. [REQ] can sombody clean up this code?
    By harryblue in forum New To Java
    Replies: 1
    Last Post: 03-18-2009, 08:40 PM
  5. clean and Build
    By bhanu in forum Eclipse
    Replies: 3
    Last Post: 07-03-2008, 01:13 PM

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •