wood burning stoves 2.0*
The moose likes Beginning Java and the fly likes Can you help me to write better code ? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of The Java EE 7 Tutorial Volume 1 or Volume 2 this week in the Java EE forum
or jQuery UI in Action in the JavaScript forum!
JavaRanch » Java Forums » Java » Beginning Java
Bookmark "Can you help me to write better code ?" Watch "Can you help me to write better code ?" New topic
Author

Can you help me to write better code ?

arun mahajan
Ranch Hand

Joined: Dec 07, 2001
Posts: 305
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
Corey McGlone
Ranch Hand

Joined: Dec 20, 2001
Posts: 3271
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


SCJP Tipline, etc.
Corey McGlone
Ranch Hand

Joined: Dec 20, 2001
Posts: 3271
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:

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
Sheriff

Joined: Dec 10, 2001
Posts: 7023
I'm a big fan of Junilu's OOCalc Tutorial over in the OO, Patterns, UML and Refactoring forum.
Good Luck.


[How To Ask Good Questions] [JavaRanch FAQ Wiki] [JavaRanch Radio]
arun mahajan
Ranch Hand

Joined: Dec 07, 2001
Posts: 305
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
Ranch Hand

Joined: Dec 20, 2001
Posts: 3271
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:

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:

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
Ranch Hand

Joined: Dec 20, 2001
Posts: 3271
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:

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:

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
 
jQuery in Action, 2nd edition
 
subject: Can you help me to write better code ?