IWETHEY v. 0.3.0 | TODO
1,095 registered users | 0 active users | 0 LpH | Statistics
Login | Create New User
IWETHEY Banner

Welcome to IWETHEY!

New Anybody want to do a Java code review?
Since this is my first major OOP project, I want to make sure I'm getting off on the right foot - could somebody with copious free time give this a once-over and let me know if I'm missing anything? This is a class project, I'm not getting paid for it, and the code is free for the ripping if anybody wants to use it.

[link|http://www.oz.net/~inthane/shipsystem.zip|Source code here.]

The project is supposed to be a data warehouse for a parts inventory, a list of people who buy stuff, their orders, and where the orders are at. Any major gaffes, please let me know.
"He who fights with monsters might take care lest he thereby become a monster. And if you gaze for long into an abyss, the abyss gazes also into you." - Friedrich Nietzsche
New Of the top of my head....
...it's really a java thing, but each class is exposing the private properties via a getter & setter. The fact that all the properties are exposed - at least indirectly - might lead one to question whether there is too much cohesion between the classes.

Not that this is really a complaint specific to your task, but it does seem to be a prevalent pattern in Java design.
New Re: Anybody want to do a Java code review?
A number of comments after a quick glance:
For all your collections you used a linked list, why? Do you need to do alot of removes? If not, an ArrayList is probably better.
Also, every time you add something you first check if it is there already, if so why not use a Set which doesn't allow duplicates?

In your collection find methods you have the following code:
foundit:
while(i.hasNext())
{

theCurrent = (CustomerPO)i.next();
if (theCustomer == theCurrent.getCustomerID())
{
if (POID == theCurrent.getCustomerPONumber())
{
break foundit;
}
}
}
if (i.hasNext())
{
return theCurrent;
}
else
{
return null;
}

A number of questions/comments:
1. Why do you need the break, once you have found the entry why not just return it?
2. What happens if the element that you find is the last one, i.hasNext() will be false but you still found the element.

You synchronized alot of methods, but not all of them, for example, you synchronized all the setAddress methods but none of the setPerson methods, do you expect to have many threads updating an address at the same time? If you do the synchronization might not help, take the following example: both Thread 1 and Thread 2 want to update Street Address 1 and 2. Thread 1 updates Address 1 then goes to sleep thread 2 updates Address 1 (Thread 1 is finished) and Address 2. Thread 1 then updates Address 2 (because thread 2 finished). The end result is that Address 1 has the new address that thread 2 entered and address 2 has the new address that thread 1 entered, in short the address is not consistent.

I don't really see where all the pieces fit together, what will drive the system?
New This is exactly why I need the review.
Only four people signed up for the course, so it got changed to a "Distance Learning" course.

This means that we have the reading list, the homework, and a professor who checks in with us about twice during the entire quarter. I've sent him my work, and nothing ever comes back.

The book I have ([link|http://www.bookpool.com/.x/dg4ptys7x1/sm/0130819344|Core Java Volume II - Advanced Features]) does everything in a very incremental way, and doesn't even really go into detail on any one particular feature of the API.

The way I'd like it:

"This is a collection. There are these types of collection. In each subsection below, we'll detail what each collection does and how it builds on the previous collection."

How they do it:

"This is a list. This is a linked list. This is a stack. This is a..."

And so on, and so on, and so on - without any real cohesion.

I need a real book.

Back to the code:

The Break I threw in there at two in the morning when I couldn't remember if returning out of a loop was a Bad Practice. I'll change that. A Set is probably is a better idea for this as well - I'm just a crusty old pointer boy who happens to like linked lists. Call me Pointerizer. ^_^

Good catch on the .hasNext function - looks like that won't work. Same with the address stuff - I'll probably stick address changes into a single function.

The front end for all this is a (still under design) GUI - hence my call for a book that actually explains AWT/Swing. I just figured out how ActionListeners work last night at about 11 (when my perception of reality was swimming dangerously close to Douglas Adams territory, so I probably have it wrong) so that part is making sense.

(FYI - I'm using the Forte community IDE - but don't worry, Malraux, I'm not using any of the integrated tools. I bashed around with the GUI design tools for about three minutes and realized I'd probably make more headway writing the !$!%%^## code by hand, at least until I understand how the damn API works.)

The problem I'm having with Java mainly links back to the "trying to take a sip from Niagra Falls" problem - there's so much out there, and they dump it on you so quickly, that I'm drowning...
"He who fights with monsters might take care lest he thereby become a monster. And if you gaze for long into an abyss, the abyss gazes also into you." - Friedrich Nietzsche
New Couple comments
Just from a conventional style thing ie what everybody does - var names and package names start with a lower case letter. Only class names are fully capitalized. I always start my member var names with an underscore so I don't have to keep going this.name = name; instead I say _name = name and I can tell at a glance whats an ivar and whats a param or temp.

That's niggly though.

From a design point of view let me ask a couple questions.

SalesOrder derives from CustomerPO. Is a CustomerPO ever of use by itself? If not, why isn't CustomerPO declared abstract? Ditto for Person and other superclasses. Also, why subclass at all? How many subtypes does CustomerPO have? Are the similarities important. Is it important to be able to deal with things at the CustomerPO level ignoring the true type of the object. If not, do away with the superclass. As a rule of thumb, BTW, I try like hell to avoid any inheritance at all in my business entity model because its a major PITA to map inheritance into a RDBMS and in general its more trouble than its worth. Inheritance in general is much overused.

New thing:
this ctor:
public Customer(String newFirstName,
String newLastName,
String newCompanyName,
String newAccountNumber,
Address newBillingAddress,
Address newShippingAddress,
String newPhoneNumber,
int newBuyerID,
double newDiscount,
boolean newTaxExempt,
String newTaxCertNumber,
String newInstructions,
String newNotes)

is an unholy nightmare and the odds of any application programmer correctly filling in a parameter list like that are roughly <number of params factorial> to 1. Better to let the developer construct the thing with a few key fields and let them set the other stuff via accessors. Its just easier to remember. If this is the only ctor, nobody is ever going to manage to create one of these things - its too hard.

You use eager initialization here:

public SalesOrder(CustomerPO newCustomerPO)
{
\tif (newCustomerPO == null)
\t{
\t setPO(new CustomerPO());
\t}
\telse
\t{
\t setPO(newCustomerPO);
\t}
}

which I assume means that you gotta have a CustomerPO. OTOH, its definitely possible to create one of these objects in some GUI and then never use it (because the user hits cancel or something), which is fine. So you throw away an object - but why throw away two? Use lazy initialization instead. Let the PO be null and create one when its asked for. ie

public CustomerPO getPO()
{
if(associatedPurchaseOrder == null) setPO(new CustomerPO());
\treturn associatedPurchaseOrder;
}

The efficiency freaks will holler "but you do that test for null over and over again! Yeah so? That test is super super cheap. In fact, I defy you to write two versions of the program and get that test's impact to show up in your performance profile.

Other question/comment. You have these CollectionOf.... things that I assume are an attempt to do typesafe collections. OK, sort of. Thing is - if you ever have to give one of these to some foreign API, your collection class is not going to be the right type. You'll have to convert. You may have to anyway but its worth a thought - what data type is going to be most convenient to pass around? A typesafe toArray might be helpful.

Other thing - you declare the implementing class to be a LinkedList. Why?

Problem #1 - the interface for the collection implementation class you've specified is too specific. Declare the ivar and all call interfaces to be simply List. Then you can tune and you only have to change the line that says new <concrete List class> to change the implementation. Never declare a variable of type concrete collection subclass - only of type abstract collection interface. Same for params.

Problem #2 - is it OK to have a Buyer appear twice in CollectionOfBuyer? Is the order of the items in the collection important? My guess is no, and no. So you really want a Set don't you. Declare the ivar of type Set and then construct some concrete Set class to initialize it. Now you get sanity checking for free against double adds.

These are just things I found in about 5 minutes - there may be other things but this is what jumps out at me.

New Thanks
Sales orders and Customer Purchase Orders are related - you can't initiate a sales order without a customer purchase order. Sales Orders must have a unique Sales Order ID, and Customer POs don't, since different customers may have ID collisions - Otherwise, they're identical.

I'll put in some other creators.

Collection issues are mainly due to poor book/unfamiliarity with language/lack of instructor assistance.

I did do some code updates last night, but forgot to replace the old version with the new - so I'll be doing that tonight, along with a (very primitive) GUI. Right now, the tendrils from the GUI are starting to reach out towards the tendrils of the data stores, but neither are seeing each other yet.
"He who fights with monsters might take care lest he thereby become a monster. And if you gaze for long into an abyss, the abyss gazes also into you." - Friedrich Nietzsche
New questions
>> Sales orders and Customer Purchase Orders are related - you can't initiate a sales order without a customer purchase order. Sales Orders must have a unique Sales Order ID, and Customer POs don't, since different customers may have ID collisions - Otherwise, they're identical. <<

A "sales order" is like a draft purchase order, no? I see no reason to use inheritance there. A purchase order is not "a kind of" sales order.

But, anyhow, why are you allowing "ID collisions"? Why have ID's if they collide? Am I missing something here? Are they internal sales orders, or external? Normally, outsiders don't see internal sales orders, except for stuff they order.

And, why have unique sales order ID's, but not unique purchase order ID's?


________________
oop.ismad.com
New Purchase Order ID is specified by the purchaser, i.e...
the customer. Customers use POs to track orders they make, the order approval and budgeting, order receiving, and eventually payment.

Customers do not coordinate their P. O. IDs among themselves to make your life simple.
Alex

Men never do evil so completely and cheerfully as when they do it from religious conviction. -- Blaise Pascal (1623-1662)
New That is an external ID
The internal system should still uniquely identify them internally. The customer's number is just an attribute that is passed on. One should not rely on it for providing unique tracking/labling.

I suppose some companies don't track them as self-standing entities, just simply associate them with the sales order or a new in-progress invoice.
________________
oop.ismad.com
Expand Edited by tablizer Nov. 10, 2001, 08:25:25 PM EST
New Yep. You got it.
Alex

Men never do evil so completely and cheerfully as when they do it from religious conviction. -- Blaise Pascal (1623-1662)
New Then why not simply keep it an attrib instd of object?
________________
oop.ismad.com
New Excellent place for code reuse.
I have two objects, both of which are almost identical, except for that one of them has to use a unique id, where the other one doesn't. Inherit, override, and reuse most of the code.

I just saved myself some time recoding the exact same thing twice.
"He who fights with monsters might take care lest he thereby become a monster. And if you gaze for long into an abyss, the abyss gazes also into you." - Friedrich Nietzsche
New Good comments
I always start my member var names with an underscore so I don't have to keep going this.name = name; instead I say _name = name and I can tell at a glance whats an ivar and whats a param or temp.

I would just add here that "this." makes it quite explicit where the variable is coming from, which is a good thing. I tend to do that, or prepend member variables with 'm' or the like. Whatever makes sense to you, but the key is to make the member variables stand out in the code explicitly. I don't tend to like underscores because they're easy to miss (especially for me because of my eye problems).

Regards,

-scott anderson
New Re: Anybody want to do a Java code review?
> Since this is my first major OOP project, I want to make sure I'm getting off on the right foot - could somebody with copious free time give this a once-over and let me know if I'm missing anything? This is a class project, I'm not getting paid for it, and the code is free for the ripping if anybody wants to use it.
>
> [link|http://www.oz.net/~inthane/shipsystem.zip|Source code here.]
>
> The project is supposed to be a data warehouse for a parts inventory, a list of people who buy stuff, their orders, and where the orders are at. Any major gaffes, please let me know.
> "He who fights with monsters might take care lest he thereby become a monster. And if you gaze for long into an abyss, the abyss gazes also into you." - Friedrich Nietzsche
If you want to know how to do any OO solutions, no matter what the language, have a look at ye olde Gamma 4 book, it's really worth it! Okay, you may not of heard of it, but you should have... It is known by non-followers as Design Patterns by Gamma and three other gadgers who know what they are talking about. I look forward to seeing you at morning Gamma worship...
     Anybody want to do a Java code review? - (inthane-chan) - (13)
         Of the top of my head.... - (ChrisR)
         Re: Anybody want to do a Java code review? - (bluke) - (1)
             This is exactly why I need the review. - (inthane-chan)
         Couple comments - (tuberculosis) - (8)
             Thanks - (inthane-chan) - (6)
                 questions - (tablizer) - (5)
                     Purchase Order ID is specified by the purchaser, i.e... - (a6l6e6x) - (4)
                         That is an external ID - (tablizer) - (3)
                             Yep. You got it. -NT - (a6l6e6x) - (2)
                                 Then why not simply keep it an attrib instd of object? -NT - (tablizer) - (1)
                                     Excellent place for code reuse. - (inthane-chan)
             Good comments - (admin)
         Re: Anybody want to do a Java code review? - (bri)

Peanut butter is "dinner" as long as you put it on a plate.
371 ms