Myujth

I have created the following view extending the JPanel class, the objective of this view is to allow the user to perform 3 CRUD operations create,delete and modify on the entity "Attribute", this entity belong to an entity User or Client, so when the user wants to perform CRUD operations it has to specify if the operations will be performed on the User Attributes or Client Attributes

The view is divided in 2 parts:

• The 1st one is for displaying Fixed Attributes from the User and
  Client entities, there's not need to perform CRUD operations on
  those.




• The 2nd one is for performing the CRUD operations on the "Additional
  Attributes".

How can I improve my class? is there some way to create less JComboBoxes?
How can I improve the look of the JScrollPane?

 package view.attributePanes;
import java.awt.Dimension;
import javax.swing.BorderFactory;
import javax.swing.JButton;
import javax.swing.JComboBox;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JScrollPane;
import javax.swing.JTextField;
import net.miginfocom.swing.MigLayout;
@SuppressWarnings("serial")
public class PaneAttributeManagement extends JPanel 
 /************ Strings **************/
 private static String types = "String", "Int", "Date", "Boolean" ;
 private static String entities = "Users", "Clients" ;
 /************ JComboBoxes **************/
 @SuppressWarnings( "unchecked", "rawtypes" )
 private JComboBox boxAttributeTypes = new JComboBox(types);
 @SuppressWarnings( "unchecked", "rawtypes" )
 private JComboBox boxAttributeTypes1 = new JComboBox(types);
 @SuppressWarnings( "unchecked", "rawtypes" )
 private JComboBox boxEntities = new JComboBox(entities);
 @SuppressWarnings( "unchecked", "rawtypes" )
 private JComboBox boxEntities1 = new JComboBox(entities);
 @SuppressWarnings( "unchecked", "rawtypes" )
 private JComboBox boxEntities2 = new JComboBox(entities);
 @SuppressWarnings( "rawtypes" )
 private JComboBox boxDeleteAttributesName = new JComboBox();
 @SuppressWarnings( "rawtypes" )
 private JComboBox boxModifyAttributesName = new JComboBox();
 /************ JTextFields **************/
 JTextField addAttributeNameTxt = new JTextField(15);
 JTextField addAttributeValueTxt = new JTextField(15);
 JTextField modifyAttributeNameTxt = new JTextField(15);
 JTextField modifyAttributeValueTxt = new JTextField(15);
 /************ JButtons **************/
 private JButton addNewAttributeBtn = new JButton("Add");
 private JButton deleteAttributeBtn = new JButton("Delete");
 private JButton modifyAttributeBtn = new JButton("Update");
 /************ Main panes **************/
 private JPanel mainPane;
 private JScrollPane mainScrollPane;
 public PaneAttributeManagement() 
 mainPane = new JPanel();
 mainPane.setLayout(new MigLayout("wrap 2", " 16 "));
 initComponents();
 
 protected void initComponents() 
 MigLayout layout = new MigLayout("wrap 2", "[grow][grow]", "[grow][grow][grow]");
 setLayout(layout);
 add(getMainScrollPane(), "cell 0 0");
 mainPane.add(getFixedAttributesPanel(), "cell 0 0,grow");
 mainPane.add(getAdditionalAttributePanel(), "cell 0 1 2 1,grow");
 
 private JScrollPane getMainScrollPane() 
 mainScrollPane = new JScrollPane(mainPane);
 mainScrollPane.setHorizontalScrollBarPolicy(JScrollPane.HORIZONTAL_SCROLLBAR_NEVER);
 mainScrollPane.setPreferredSize(new Dimension(470, 400));
 return mainScrollPane;
 
 private JPanel getFixedAttributesPanel() 
 JPanel panel = new JPanel();
 panel.setBorder(BorderFactory.createTitledBorder("Fixed Attributes"));
 panel.setLayout(new MigLayout("wrap 2", " 16 "));
 panel.add(getFixedUserAttributesPanel());
 panel.add(getFixedClientAttributesPanel());
 return panel;
 
 private JPanel getFixedUserAttributesPanel() 
 JPanel panel = new JPanel();
 panel.setBorder(BorderFactory.createTitledBorder("User"));
 panel.setLayout(new MigLayout("wrap 2", " 16 "));
 panel.add(new JLabel("1."), "right");
 panel.add(new JLabel("User ID"));
 panel.add(new JLabel("2."), "right");
 panel.add(new JLabel("Name"));
 panel.add(new JLabel("3."), "right");
 panel.add(new JLabel("Surname"));
 panel.add(new JLabel("4."), "right");
 panel.add(new JLabel("Password"));
 panel.add(new JLabel("5."), "right");
 panel.add(new JLabel("Clients owned"));
 return panel;
 
 private JPanel getFixedClientAttributesPanel() 
 JPanel panel = new JPanel();
 panel.setBorder(BorderFactory.createTitledBorder("Client"));
 panel.setLayout(new MigLayout("wrap 2", " 16 "));
 panel.add(new JLabel("1."), "right");
 panel.add(new JLabel("Client ID"));
 panel.add(new JLabel("2."), "right");
 panel.add(new JLabel("Name"));
 return panel;
 
 private JPanel getAdditionalAttributePanel() 
 JPanel panel = new JPanel();
 panel.setBorder(BorderFactory.createTitledBorder("Additional Attributes"));
 panel.setLayout(new MigLayout("wrap 1", " 16 "));
 panel.add(getAddAttributePanel());
 panel.add(getModifyAttributePanel());
 panel.add(getDeleteAttributePanel());
 return panel;
 
 private JPanel getAddAttributePanel() 
 JPanel panel = new JPanel();
 panel.setBorder(BorderFactory.createTitledBorder("Add Attribute"));
 panel.setLayout(new MigLayout("wrap 2", "16", ""));
 panel.add(new JLabel("For:"));
 panel.add(boxEntities);
 panel.add(new JLabel("Name:"));
 panel.add(addAttributeNameTxt);
 panel.add(new JLabel("Type:"));
 panel.add(boxAttributeTypes);
 panel.add(new JLabel("Value:"));
 panel.add(addAttributeValueTxt);
 panel.add(addNewAttributeBtn, "cell 1 4, right");
 return panel;
 
 private JPanel getDeleteAttributePanel() 
 JPanel panel = new JPanel();
 panel.setBorder(BorderFactory.createTitledBorder("Delete Attributes"));
 panel.setLayout(new MigLayout("wrap 2", "16", ""));
 panel.add(new JLabel("From:"));
 panel.add(boxEntities1);
 panel.add(new JLabel("Name:"));
 panel.add(boxDeleteAttributesName);
 panel.add(deleteAttributeBtn, "cell 2 3,right");
 return panel;
 
 private JPanel getModifyAttributePanel() 
 JPanel panel = new JPanel();
 panel.setBorder(BorderFactory.createTitledBorder("Modify Attributes"));
 panel.setLayout(new MigLayout("wrap 2", "16", ""));
 panel.add(new JLabel("From:"));
 panel.add(boxEntities2);
 panel.add(new JLabel("Name:"));
 panel.add(boxModifyAttributesName);
 panel.add(new JLabel("New name:"));
 panel.add(modifyAttributeNameTxt);
 panel.add(new JLabel("New type:"));
 panel.add(boxAttributeTypes1);
 panel.add(new JLabel("New value:"));
 panel.add(modifyAttributeValueTxt);
 panel.add(modifyAttributeBtn, "cell 1 5, right");
 return panel;
 
 public String getAddAttributeNameTxt() 
 return addAttributeNameTxt.getText();
 
 public String getAddAttributeValueTxt() 
 return addAttributeValueTxt.getText();
 
 public String getModifyAttributeNameTxt() 
 return modifyAttributeNameTxt.getText();
 
 public String getModifyAttributeValueTxt() 
 return modifyAttributeValueTxt.getText();
 

Quickfire opinions:

• Swing is deprecated. If you have the choice: do not use swing!
  


• Empty lines are a useful thing.
  It's pretty conventional to use empty lines between members and around the import section of code.
  It helps delineate logically connected sections of code fromone another.


• The use of extends JPanel is something that many swing tutorials do.
  It's also one of the worst things to teach people new to the UI framework.
  Especially for UI the rule of thumb "Composition over Inheritance" applies.
  The calling code for the UI should not care whether the UI is a Panel or a CLI interface or even some web interface.
  Use MVP or MVC to write properly encapsulated and abstracted UIs that can be easily maintained or replaced.


• There should be no reason whatsoever in a GUI to ignore the warnings rawtypes and serial.
  Both of these warnings point to an underlying problem in the modelling of the domain and a lack of familiarity with the GUI framework.
  I do appreciate the commitment to treating warnings as important though :)


• Systems hungarian notation should die.
  Don't suffix or prefix members with indications of their type like Txt box or Btn.
  If you need that to have unique names, your UI is not properly separating model from presentation.


• Use final wherever possible to indicate a lack of modifiability.


• Use the smallest access scope possible.
  All text fields in the view should be private


• Section headers in comments are a code-smell.
  Using them implies that you're not using sufficiently communicative names for members.

Not so quickfire changes:

Use compile-time guarantees for correctness wherever possible:

private static final String types = String.class.getSimpleName(), Integer.class.getSimpleName(),
 Date.class.getSimpleName(), Boolean.class.getSimpleName() ;
private static final String entities = User.class.getSimpleName(), Client.class.getSimpleName() ;

Note that this does change which strings are actually displayed.
What you're conveniently ignoring in the code presented is challenges posed by localization of your user interface and the application domain.
The code can't deal with your sales team getting the idea of selling the software in any non-englishspeaking country.
That's an issue, because it detracts from the value of the product on one hand and additionally adds technical debt and a suboptimal approach to exposing the domain to the user.

I also noticed that you're declaring all the members and variables you use upfront. Only a little portion of those need to be explicitly maintained, though. The only things that you should "cache" in members are the panels that you're returning from the side-effecting getters and the members you need to expose for presentation purposes. The fact that your getters create a new JPanel every time they are invoked just made me go into a rant to a friend sitting next to me for 15 minutes!!!! DON'T DO THAT!

Side-effecting getters result in a change in reference and operation semantics that make it near-impossible to correctly refactor any code that's interfacing with your UI without fully understanding said UI.

Consider the following simplified code example:

Foo someFoo = new Foo();
Bar someBar = someFoo.getBar();
someBar.doSomething();
someBar.doSomethingElse();

Assuming the usual semantics for standard getters in java, I'd have expected the following code to have the same effect:

Foo someFoo = new Foo();
someFoo.getBar().doSomething();
someFoo.getBar().doSomethingElse();

The code is violating these assumptions and semantics of getters. This makes it near-unmaintainable IMO.
Instead of naming these get*, you should call them create*, since every invocation of these methods returns a specifically created value.

I'm happy that these are just private factory methods encapsulating the creation of components, but it's still something that you shouldn't do!

CRUD is actually written C.R.U.D.

The overall UI experience presented here is badly designed.
The UI realizes multiple fundamentally different operations in the same View.
Updating and Adding an Attribute are fundamentally different.
Don't handle them in the same UI.

I'd be okay with modification and deletion to be on the same view, but then you should present a proper ListView of all attributes that the user can modify.

As it stands the class you presented does not expose any facilities for invoking code to set values (which are read from a database).
You're not exposing any reasonable way to prefill the ModifyAttributePanel.

I'm kinda sorry, but this code is not even close to production ready.
Please go back to the drawing board and reconceptualize how the requirements should be translated into a view.

This brings me to the end of today's post in the category "Vogel Rants".