Can you help me to write better code ?

[arun mahajan]

I have written following simple program but seems not satisfied. I would like to ask following questions:-

1. Will it take less memory?
2. Is there better way of declaring variables?
3. Is this monolithic code for ur actionPerformed() OK? or can I adopt some better way for same?
4. Can some body make changes,if required, and let me know how to make it better efficient?

regards,
Arun

package test;
import java.awt.*;
import java.awt.event.*;
import javax.swing.*;
public class test extends JFrame
implements ActionListener
{
JButton toolbutton;
JToolBar toolbar;
boolean toolbar_flag = true;
makePanel mp = new makePanel();
JMenuBar menubar;
JMenu menu;
JMenuItem menuItem;

public test()
{
Containercont = getContentPane();
cont.setBackground(new Color(140,199,222));
cont.setFont(new Font("arial",Font.BOLD,11));
cont.setLayout(null);
buildMenu();
        //setting frame size
setSize(450,400);
//setting toolbar
toolbar = new JToolBar();
toolbar.setBackground(new Color(140,199,222));
addToolBarButtons(toolbutton);
toolbar.setBounds(-2,-2,290,30);
toolbar.setFloatable(false);
cont.add(toolbar);
mp.setBounds(0,25,450,375);
cont.add(mp);
mp.cont();
//setting windows listeners
addWindowListener(new WindowAdapter() {
public void windowClosing(WindowEvent e) {
System.exit(0);
}
        });
        //setting frame to a fix size
addComponentListener(new ComponentAdapter() {
public void componentResized(ComponentEvent e) {
setSize(450,400);
}
       });
      }
      private void buildMenu()
      {
menubar = new JMenuBar();
setJMenuBar(menubar);
//Build File Menu
menu = new JMenu("File");
menu.setMnemonic(KeyEvent.VK_F);
menubar.add(menu);
//Items in File Menu
menuItem = new JMenuItem("Change User/Login",KeyEvent.VK_C);
menuItem.setAccelerator(KeyStroke.getKeyStroke(KeyEvent.VK_1, ActionEvent.ALT_MASK));
        menuItem.addActionListener(this);
        menu.add(menuItem);
        //adding separator
menu.addSeparator();
menuItem = new JMenuItem("Clear Screen",KeyEvent.VK_S);
menuItem.setAccelerator(KeyStroke.getKeyStroke(KeyEvent.VK_2, ActionEvent.ALT_MASK));
        menuItem.addActionListener(this);
        menu.add(menuItem);
        //adding separator
menu.addSeparator();
menuItem = new JMenuItem("Exit",KeyEvent.VK_X);
        menuItem.addActionListener(this);
        menu.add(menuItem);
  }
protected void addToolBarButtons(JButton jbutton)
{
jbutton = new JButton(new ImageIcon("images/buttons/change_user.gif"));
jbutton.setToolTipText("Change User/Login");
jbutton.addActionListener(this);
toolbar.add(jbutton);
jbutton = new JButton(new ImageIcon("images/buttons/clear.gif"));
jbutton.setToolTipText("Clear Screen");
jbutton.addActionListener(this);
toolbar.add(jbutton);
jbutton = new JButton(new ImageIcon("images/buttons/exit.gif"));
jbutton.setToolTipText("Exit");
jbutton.addActionListener(this);
toolbar.add(jbutton);
}
public void actionPerformed(ActionEvent evt)
{
Object o = evt.getSource();
String s= ""+o;
if(s.indexOf("JMenuItem")>0)
{
s="";
JMenuItem source = (JMenuItem)(evt.getSource());
s = ""+source.getText();
}
else if(s.indexOf("JButton")>0)
{
s="";
JButton source = (JButton)(evt.getSource());
s = ""+source.getToolTipText();
}
//for File Menus
if("Change User/Login".compareToIgnoreCase(s)==0)
{
mp.login(this);
}
else if("Clear Screen".equals(s))
{
mp.clear();
}
else if("Exit".equals(s))
{
System.exit(1);
}
else if((evt.getActionCommand()=="Login"))
{
//calling wait message
mp.clearWithBlue();
//gathering userinformation
String id = mp.pid.getText();
char[] cpwd = mp.ppwd.getPassword();
String pwd="";
for(int i=0;i<cpwd.length;i++)
pwd += ""+cpwd[i];
boolean boxstate = mp.jcb.isSelected();
String xx=""+id+"$"+pwd+"$"+boxstate+"$";
System.out.println("*******"+xx+"*******");
}
}
public static void main(String args[])
{
test ls=new test();
ls.setTitle("test!!");
ls.setVisible(true);
}
}
//class to  handle all the components
class makePanel extends JPanel
{
JLabel label3=null;
JTextField pid =null;
JPasswordField ppwd=null;
JCheckBox jcb = null;
JButton  go = new JButton(new ImageIcon("images/go.gif"));
public makePanel()
{
super();
setLayout(null);
setBackground(new Color(140,199,222));
}
public void cont()
{
JLabel back = new JLabel(new ImageIcon("images/mbg.jpg"));
back.setBounds(112,107,338,218);
add(back);
}
public void login(ActionListener ae)
{
removeAll();
JLabel id = new JLabel();
id.setBounds(142,95,60,20);
id.setText("Login :");
id.setForeground(new Color(255,255,255));
add(id);
JLabel pwd = new JLabel();
pwd.setBounds(115,125,80,20);
pwd.setText("Password :");
pwd.setForeground(new Color(255,255,255));
add(pwd);
pid = new JTextField();
pid.setBounds(200,95,100,20);
add(pid);
ppwd = new JPasswordField();
ppwd.setBounds(200,125,100,20);
ppwd.setEchoChar('*');
add(ppwd);
go.setBounds(295,125,50,20);
go.setForeground(new Color(255,255,255));
go.setOpaque(false);
go.setBorder(null);
go.setActionCommand("Login");
add(go);
go.addActionListener(ae);
jcb = new JCheckBox();
jcb.setBounds(150,155,15,15);
jcb.setBackground(new Color(8,142,206));
add(jcb);
JLabel remPwd = new JLabel();
remPwd.setBounds(170,155,150,20);
remPwd.setText("Remember My ID");
remPwd.setForeground(new Color(255,255,255));
add(remPwd);
JLabel back = new JLabel(new ImageIcon("images/pbg.gif"));
back.setBounds(105,65,231,142);
add(back);
cont();
repaint();
}
public void clear()
{
removeAll();
cont();
repaint();
}
public void clearWithBlue()
{
removeAll();
label3= new JLabel(new ImageIcon("images/wait.gif"));
label3.setBounds(110,75,200,120);
add(label3);
JLabel back = new JLabel(new ImageIcon("images/pbg.gif"));
back.setBounds(105,65,231,142);
add(back);
cont();
repaint();
}
}
  

[Corey McGlone]

Ok, here's a couple things I'd like to point out.
Your code is too long (kinda). Obviously, you need all of the code here, but you've got too much of it grouped into big functions. Try to keep your functions short and to the point. Some of the methods, like login, can be broken down into smaller methods. This will help keep your code more maintainable, readable, and flexible. Well-focused methods are a good thing. Try not to let them get too long.
It's often useful to use another class as an ActionListener rather than using the panel itself. Obvisouly, it works, but you might want to think about using an inner class or just a separate top-level class to keep the event handling code separated from the code for building and displaying the form. Check out this thread where this was discussed.
Try those couple of things and see if you like your code any better.
Corey

[Corey McGlone]

One other thing I noticed. I see that you're creating a lot of new Color objects, even though they're the same color. Create just one Color object for each color and then reuse it. For example, do this:

public void login(ActionListener ae)
{
   Color c = new Color(255, 255, 255);
   ...
   id.setForeground(c);
   ...
   pwd.setForeground(c);
   ...
   go.setForeground(c);
   ...
   remPwd.setForeground(c);
}

If you're using these same objects in multiple methods, you might even want to think about making this Color object a member variable.
Corey

[Dirk Schreckmann]

I'm a big fan of Junilu's OOCalc Tutorial over in the OO, Patterns, UML and Refactoring forum.
Good Luck.

[arun mahajan]

It's often useful to use another class as an ActionListener rather than using the panel itself. Obvisouly, it works, but you might want to think about using an inner class or just a separate top-level class to keep the event handling code separated from the code for building and displaying the form

I am sorry to ask this but I have little experience on inner classes. can you rewrite a part of actionPerformed method to let me understand it better.

Making my login method shorter to make it more readable and maintainable

I could not grasp this also. because in this method I am just decalring all labels and other compontnets required by this. How to make it more efficient? can you re-write a code snippent for me.
Hope I am not putting you pepople on extra work.
Thanks once again for help extending.
regards,
Arun :roll:
[ April 03, 2002: Message edited by: arun mahajan ]

[Corey McGlone]

Okay, first, let's see what this looks like if we use a couple inner classes to handle the ActionEvent. Originally, you had the Panel itself acting as the ActionEventListener. I'm going to make a couple of inner classes that can do the listening for us. One goal you should always have in mind is the "separation of concerns." You want to keep your code to display your screen and your code to handle events separate. That way, if you have to change the way your screen looks, you don't necessarily have to change the way events are handled. This can greatly increase application flexibility and maintainability.
I'd rewrite your test class to be something like this:

public class test extends JFrame
{
   ...
   private class MyMenuListener implements ActionListener
   {
      public void actionPerformed(ActionEvent e)
      {
         // Do your event handling for menu items here...
      }
   }
 
   private class MyButtonListener implements ActionListener
   {
      public void actionPerformed(ActionEvent e)
      {
         // Do your event handling for buttons here...
      }
   }
 
   ...
 
   private void buildMenu()
   {
      ...
      menuItem.addActionListener(new MyMenuListener());
      ...
   }
 
   protected void addToolBarButtons(JButton jbutton)
   {
      ...
      jbutton.addActionListener(new MyButtonListener());
      ...
   }
}

You see, you can declare a class as a member variable just as if you were declaring any other member. This is known as an inner class. Do a search in this forum or the Programmer Certification Forum to find gobs of information on inner classes. By creating two separate inner classes, you can separate the action events that you'll be handling. The class MyMenuListener only has to worry about MenuItem actions - it doesn't have to worry about Button actions because those will be sent to and instance of the MyButtonListener class.
Next, for the extra methods. Just think "divide and conquer." You have a method called login, which is pretty long. In that method, you're repeatedly doing the same things, just to different objects. Let's make another method:

private void configComponent(Component c, Rectangle bounds, String text, Color color)
{
   c.setBounds(bounds);
   c.setText(text);
   c.setForeground(color);
}
 
public void login(ActionListener ae)
{
   removeAll();
   Color c = new Color(255,255,255);
   JLabel id = new JLabel();
   configComponent(id, new Rectangle(142, 95, 50, 20), "Login :", c);
   add(id);
   JLabel pwd = new JLabel();
   configComponent(pwd, new Rectangle(115,125,80,20), "Password :", c);
   add(pwd);
   ...
}

As you can see, this makes your code much shorter and more readable. You don't need to rewrite code to do the same thing repeatedly, just write a method and call it over and over again.
One other thing, you might want to look at using a LayoutManager, rather than setting the bounds for all of your components by hand. It might make your life a lot easier.
I hope that helps,
Corey
[ April 03, 2002: Message edited by: Corey McGlone ]

[Corey McGlone]

Okay, first, let's see what this looks like if we use a couple inner classes to handle the ActionEvent. Originally, you had the Panel itself acting as the ActionEventListener. I'm going to make a couple of inner classes that can do the listening for us. One goal you should always have in mind is the "separation of concerns." You want to keep your code to display your screen and your code to handle events separate. That way, if you have to change the way your screen looks, you don't necessarily have to change the way events are handled. This can greatly increase application flexibility and maintainability.
I'd rewrite your test class to be something like this:

public class test extends JFrame
{
   ...
   private class MyMenuListener implements ActionListener
   {
      public void actionPerformed(ActionEvent e)
      {
         // Do your event handling for menu items here...
      }
   }
 
   private class MyButtonListener implements ActionListener
   {
      public void actionPerformed(ActionEvent e)
      {
         // Do your event handling for buttons here...
      }
   }
 
   ...
 
   private void buildMenu()
   {
      ...
      menuItem.addActionListener(new MyMenuListener());
      ...
   }
 
   protected void addToolBarButtons(JButton jbutton)
   {
      ...
      jbutton.addActionListener(new MyButtonListener());
      ...
   }
}

You see, you can declare a class as a member variable just as if you were declaring any other member. This is known as an inner class. Do a search in this forum or the Programmer Certification Forum to find gobs of information on inner classes. By creating two separate inner classes, you can separate the action events that you'll be handling. The class MyMenuListener only has to worry about MenuItem actions - it doesn't have to worry about Button actions because those will be sent to and instance of the MyButtonListener class.
Next, for the extra methods. Just think "divide and conquer." You have a method called login, which is pretty long. In that method, you're repeatedly doing the same things, just to different objects. Let's make another method:

private void configComponent(Component c, Rectangle bounds, String text, Color color)
{
   c.setBounds(bounds);
   c.setText(text);
   c.setForeground(color);
}
 
public void login(ActionListener ae)
{
   removeAll();
   Color c = new Color(255,255,255);
   JLabel id = new JLabel();
   configComponent(id, new Rectangle(142, 95, 50, 20), "Login :", c);
   add(id);
   JLabel pwd = new JLabel();
   configComponent(pwd, new Rectangle(115,125,80,20), "Password :", c);
   add(pwd);
   ...
}

As you can see, this makes your code much shorter and more readable. You don't need to rewrite code to do the same thing repeatedly, just write a method and call it over and over again.
One other thing, you might want to look at using a LayoutManager, rather than setting the bounds for all of your components by hand. It might make your life a lot easier.
I hope that helps,
Corey