Please take the below comments in the form of constructive criticism, which is what I read your post as asking for. Some of the comments are specifically about code errors and others are just idioms or comments about writing 'safer' code.
Tyler Nichol wrote:I need to access certain lines from my file via diverArray diver objects. I am trying to store names and scores of the divers
into the objects if that makes sense. I am newer to java, and have trouble understanding arrays and objects. If you have any
input on my code that would be very helpful. I am not looking for direct answers, just some direction for the new guy.
Attached is my Input.java class where my arrays and objects are created, and my Diver class where the names and scores will be stored.
1) My main question is really whether I am declaring my variables and objects correctly for the arrays.
So tell us this, what happens when you compile the code? What happens when you try to run it?
Tyler Nichol wrote:Thank you for your input!
This is my first point of concern. Why do you have this variable? Is it used?
Another point that I like to make is this: When possible, use final variables, its just safer. Do you expect the firstName and lastName variables to change after the constructor? If not, then make them final. You probably can't do the same thing with scores, unless you know that the
length of the scores array should never change.
One final note: the finalScore seems like a calculated value. It might be that you don't need to have a variable to hold that value. If you do, then make sure you keep it up to date by re-calculating it whenever scores get set.
Tyler Nichol wrote:
Here you set only the first value in the scores array. There are a couple of problems with this: first, you never initialized the this.scores array, so it is null, and you will probably get an exception. Secondly, arrays in Java are not like arrays in C/C++ when you assign the 'head' of the this.scores to a value, the rest of the values don't come along - arrays aren't just a pointer to the start of a sequence. You have to do one of the following:
* Assign the this.scores refrence to the same array referred to by the local variable scores. For example, this.scores = scores; This is probably the closest thing to what you have in your code.
* Assign each index in the this.scores array the value of the same index in the local variable scored. Note you first have to initialize the this.scores variable to a double array of the correct size, and that the assignment is probably best to be done in a for loop. Also note that, although more code and slower than the previous option, this option is the safest because no code outside the class can modify the Diver's score unexpectedly.
Tyler Nichol wrote:
Before, when I said that I like to make variables final, that would mean that I generally don't like the setNNN() methods, unless you think the values can and should change during the course of the Object's life. Does it make sense that the Diver's name change? If it does, then keep these set methods and don't make the variables final. If not then get rid of the setNNN methods and make the variables final.
Also, what is the point of passing in the Diver Object?
Tyler Nichol wrote:
This returns just the first score in the scores array. If this is what you want, then ok, if not then you need to return the full array, and make the return type a double array, not just a double. Also I will interject here more about code safety: I often will make a duplicate array, fill it with the values of this.scores and return the duplicate. Again, this is more work and takes longer, but it protects the inter data from manipulation outside the class.
Tyler Nichol wrote:
The comments I made in the constructor about the scores array and assignment apply here as well.
Tyler Nichol wrote:
There is a chance that the finalScore has never been set, and so this method could return 0.0 even if scores have been assigned. I would consider this an 'inconsistent state.' If the final score is dependent and calculated from the scores, then it should be calculated either in the get method, or it should be calculated when you set the scores.
Tyler Nichol wrote:
I would be a little worried about this method. If the finalScore is calculated from the scores, then this method should handle the calculation itself, and do so in a way that prevents an inconsistent state. Relying on an external source to do calculations for the data this class holds partially circumvents the purpose of using classes. If at all possible, get rid of this set method, and make the calculation in-class, and at a point where there can be no inconsistent state.
This assumes the finalScore is calculated from scores, though. If it is not, or if it relies on other data that this class does not have access to then these points are moot.
Tyler Nichol wrote:
