|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
. The code is fine. In other words, there's nothing horrible in it. That said, here's what I would change:
[OCP 17 book] | [OCP 11 book] | [OCA 8 book] [OCP 8 book] [Practice tests book] [Blog] [JavaRanch FAQ] [How To Ask Questions] [Book Promos]
Other Certs: SCEA Part 1, Part 2 & 3, Core Spring 3, TOGAF part 1 and part 2
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.
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.
:-)
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.
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.
Jeanne Boyarsky wrote:- findApplication() and matchApplication() do almost the same thing. Why are there two? Can one call another?
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.
[OCP 17 book] | [OCP 11 book] | [OCA 8 book] [OCP 8 book] [Practice tests book] [Blog] [JavaRanch FAQ] [How To Ask Questions] [Book Promos]
Other Certs: SCEA Part 1, Part 2 & 3, Core Spring 3, TOGAF part 1 and part 2
|
Let nothing stop you! Not even this tiny ad:
We need your help - Coderanch server fundraiser
https://coderanch.com/wiki/782867/Coderanch-server-fundraiser
|
