File APIs for Java Developers
Manipulate DOC, XLS, PPT, PDF and many others from your application.
http://aspose.com/file-tools
The moose likes Java in General and the fly likes Code review Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Java » Java in General
Bookmark "Code review" Watch "Code review" New topic
Author

Code review

Mike Lipay
Ranch Hand

Joined: Sep 11, 2007
Posts: 171
I've completed the first programming assignment in my class, we were to pick something that would be meaningful and useful to ourselves. I decided to write a program registry to replace an old one I had on a File Maker DB. The code is below, in two sections, the first section is the main application, the second is an application object. While the code works I'm sure it is not pretty and efficient. I'd like to hear from those of you with more experience: what do you think of the code, and how would you modify it?

Just as background, I am an old-time mainframe programmer trying to learn, not just a new language, but a completely new paradigm. ie, this old dog is trying to learn a few new tricks.

Thanks for your help.

Mike








BTW, I do have one platform-related question. At home I code on a Mac, at work on a Windows, the one difference between the two running applications is that the icon doesn't appear on the Mac window. Is this typical for Mac, or did I do something wrong?
John de Michele
Rancher

Joined: Mar 09, 2009
Posts: 600
Mike:

a minor nitpick, but you can eliminate these two lines:

You've already got an 'import java.io.*" statement, so a line for PrintWriter isn't needed. For the other statement, you never have to import classes that are in the java.lang package. Java automatically includes them for you.

Lines 41 and 212 in your second class: Both of those methods have a large number of similar arguments that are passed to them. You might consider creating a small, bean-type (just getters and setters) class to encapsulate the arguments. Passing a single object to a method is a lot easier than trying to keep track of which of the nine String arguments is which.

John.
Paul Clapham
Bartender

Joined: Oct 14, 2005
Posts: 18541
    
    8

A lot of code reviews would highlight the use of excessively long lines of code. The problem with those is that they are difficult to read because the reader has to scroll right and left to see the whole line. And if you're scrolling down to look for something, it's easier to miss that something if it's off the right-hand side of the screen.

In some places you've used excessively long lines of code for a reason, namely to keep related code together and to highlight similarities. But in other places you just have long lists of parameters. Those could be neatly reformatted onto multiple lines.
Jeanne Boyarsky
internet detective
Marshal

Joined: May 26, 2003
Posts: 30382
    
150

Mike,
Welcome to Java . The code is fine. In other words, there's nothing horrible in it. That said, here's what I would change:

- the frame class has a really long constructor. Anything you can do to make it shorter would make it easier to read - extract classes out, extract code into methods. Or have the action listener call a method that does the work.
- findApplication() and matchApplication() do almost the same thing. Why are there two? Can one call another?
- As mentioned above, I'm not a big fan of lines of with multiple statements either. "{ currentRecord = 0; return null; } "
- in saveApplications(), if you can't create a file, you print the exception and then try to use that file (which is null) to write data too. I think this method should be in one big try/catch block since it doesn't make sense to try later steps if earlier steps fail. The same idea for loadApplications()


[Blog] [JavaRanch FAQ] [How To Ask Questions The Smart Way] [Book Promos]
Blogging on Certs: SCEA Part 1, Part 2 & 3, Core Spring 3, OCAJP, OCPJP beta, TOGAF part 1 and part 2
Max Rahder
Ranch Hand

Joined: Nov 06, 2000
Posts: 177
I'm sorry I didn't take the time to review the code in detail, but one thing jumps out at me: I suggest avoiding multiple try..catch blocks. The point of a try block is to group the set of statements that should succeed or fail as a whole. For example, in method saveApplications(), if you fail to read the first file is there any way to recover before subsequently running writeObject()? If not, there's no point in separating the call into it's own try..catch. Instead, group all the statements that must success together in a single try block. Review all your try..catch blocks to see which should be done as a single larger try block.

:-)
Mike Lipay
Ranch Hand

Joined: Sep 11, 2007
Posts: 171
John de Michele wrote:Mike:

a minor nitpick, but you can eliminate these two lines:
[code]Lines 41 and 212 in your second class: Both of those methods have a large number of similar arguments that are passed to them. You might consider creating a small, bean-type (just getters and setters) class to encapsulate the arguments. Passing a single object to a method is a lot easier than trying to keep track of which of the nine String arguments is which.

John.


I make the first change, but I don't understand the second. What are 'beans'? Can you post a sample of bean code?
Mike Lipay
Ranch Hand

Joined: Sep 11, 2007
Posts: 171
Max Rahder wrote:I'm sorry I didn't take the time to review the code in detail, but one thing jumps out at me: I suggest avoiding multiple try..catch blocks. The point of a try block is to group the set of statements that should succeed or fail as a whole. For example, in method saveApplications(), if you fail to read the first file is there any way to recover before subsequently running writeObject()? If not, there's no point in separating the call into it's own try..catch. Instead, group all the statements that must success together in a single try block. Review all your try..catch blocks to see which should be done as a single larger try block.

:-)


I took your advice, see below, is this good, or should I combine the two remaining try...catch blocks as well?
Mike Lipay
Ranch Hand

Joined: Sep 11, 2007
Posts: 171
Paul Clapham wrote:A lot of code reviews would highlight the use of excessively long lines of code. The problem with those is that they are difficult to read because the reader has to scroll right and left to see the whole line. And if you're scrolling down to look for something, it's easier to miss that something if it's off the right-hand side of the screen.

In some places you've used excessively long lines of code for a reason, namely to keep related code together and to highlight similarities. But in other places you just have long lists of parameters. Those could be neatly reformatted onto multiple lines.



Did that, it is easier to find fields, thanks. I became used to just doing a search when I wanted to find a particular variable, but scanning for a particular block of code is easier now.
Mike Lipay
Ranch Hand

Joined: Sep 11, 2007
Posts: 171
Jeanne Boyarsky wrote:
- the frame class has a really long constructor. Anything you can do to make it shorter would make it easier to read - extract classes out, extract code into methods. Or have the action listener call a method that does the work.


I'm not sure how to do this, as all of the fields are needed for some of the methods. Can you provide example code?


Jeanne Boyarsky wrote:- findApplication() and matchApplication() do almost the same thing. Why are there two? Can one call another?


Good catch. Actually, the find is no longer needed, it was leftover code from when this was a command-line application (before GUI was introduced) I can remove it now.
John de Michele
Rancher

Joined: Mar 09, 2009
Posts: 600
Mike:

Here's an example:

In your case, since you're dealing with nine Strings, you'll probably just want to stick with the no-argument constructor.

John.
Jeanne Boyarsky
internet detective
Marshal

Joined: May 26, 2003
Posts: 30382
    
150

Mike Lipay wrote:
Jeanne Boyarsky wrote:
- the frame class has a really long constructor. Anything you can do to make it shorter would make it easier to read - extract classes out, extract code into methods. Or have the action listener call a method that does the work.


I'm not sure how to do this, as all of the fields are needed for some of the methods. Can you provide example code?
The extract methods part is easy - just stick some of the cod ein a method with a name. Since it is an instance method it has access to the fields.

For the action listener, you can use a "trick" and pass the current class in. For example:

and



I like creating the data transfer object approach recommended above where you have the fields with getters and setters.
 
Consider Paul's rocket mass heater.
 
subject: Code review