aspose file tools*
The moose likes Beginning Java and the fly likes Could you give me your opinion on whether my code is okay Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Java » Beginning Java
Bookmark "Could you give me your opinion on whether my code is okay" Watch "Could you give me your opinion on whether my code is okay" New topic
Author

Could you give me your opinion on whether my code is okay

Mark Toddd
Greenhorn

Joined: Jan 05, 2010
Posts: 11
Hi all.

I'd like to get some opinions on whether this is okay in terms of format and placement of code.
I wasn't sure whether i should declare my classes at the top of the code or in another position.... ?




Thanks


"What you talkin' bout Willis?"
salvin francis
Ranch Hand

Joined: Jan 12, 2009
Posts: 928

Why are you writing your entire code in main ?


My Website: [Salvin.in] Cool your mind:[Salvin.in/painting] My Sally:[Salvin.in/sally]
Mark Toddd
Greenhorn

Joined: Jan 05, 2010
Posts: 11
Hi

Because I'm a noob and I don't know better
How would you suggest I write it better?

Cheers
salvin francis
Ranch Hand

Joined: Jan 12, 2009
Posts: 928

Well, java is all about reusability, portability and crap like that

The reason i asked you that question was that i needed to understand why you thought of doing so. You have written a GUI program, trust me, you are not a 'noob'

here is what you probably need to do:
Grab a cup of coffee

See, your class "FirstFrame" indicates a frame being used, here is what i would have done:




Also the ActionListener code, being simple can be tackled in this manner:



where messageClicked() is a method in MainFrame class
next, I would prefer menubar, mypanel, etc. to be instance variables instead of local variables.

try these changes and post the new code here ...
Ernest Friedman-Hill
author and iconoclast
Marshal

Joined: Jul 08, 2003
Posts: 24187
    
  34

Salvin: your suggested changes add two new essentially empty methods, and make the one long method substantially longer by folding the named listener classes into the code. So I'd say your changes make the program nontrivially worse, and in no way better.

Mark: What would I do, specifically, to improve the code we're looking at?


  • There are some repeated calls (setVisible(), setSize()); you can remove the duplicates.
  • You want to defer calling setVisible() until the whole GUI is assembled (i.e., do it last).
  • You could break related chunks of code out into sensible-named methods; i.e., I see about six lines that could be moved into a method named setUpMenuBar(), and ten that could become setUpContentPanel() .
  • Many people recommend only importing the classes you use, rather than using "*" to import a whole package.


Declaring the nested classes at the top of the class is fine. I often put them at the end; either way is probably better than randomly distributing them.


[Jess in Action][AskingGoodQuestions]
salvin francis
Ranch Hand

Joined: Jan 12, 2009
Posts: 928

Ernest Friedman-Hill wrote:So I'd say your changes make the program nontrivially worse, and in no way better.



I havent even written the program, just suggested changes as a start.
Maybe you could have pointed points you agree with me and points you do not agree with me [preferably with reasons] thus giving our friend Mark sman a better idea about java.


1. Do you agree that he should not have written his entire program in main ?
2. Do you agree with the init() method ?
3. My approach was to separate listener logic with the code using methods such as messageClicked and maybe closeFrame using anonymous class to do the calling
this, in my opinion helps when the program has a change in logic, programmer has to just change the implementation of the method itself.


I do not fully understand your point of making "one long method substantially longer"
Gregg Bolinger
GenRocket Founder
Ranch Hand

Joined: Jul 11, 2001
Posts: 15299
    
    6

Another thing to note is that UI code must execute on the Event Dispatching Thread (EDT). The main() will get started in a special thread the VM kicks off which is NOT on the EDT. So we need should make sure that it happens on the EDT by using the SwingUtilties.invokeLater() method like so...



Also note that it is becoming standard practice to not inherit from JFrame since so little is generally done with the JFrame itself you're better off not coupling your main class to it. But for learning purposes it's not that big a deal.


GenRocket - Experts at Building Test Data
 
I agree. Here's the link: http://aspose.com/file-tools
 
subject: Could you give me your opinion on whether my code is okay