Claude Moore wrote:Carefully have a look at your equals() methods. What may be wrong with them ?
Campbell Ritchie wrote:Start by getting rid of the long lines, which make the code very difficult to read. Think how much worse it would be on paper. We suggest BSD/Allman indentation here, but if you are using K&R indentation, separate all { from what precedes them with a few spaces, and also divide }else{ with spaces.
No need to be sorry; that is one of the things you can expect if you ask for code to be reviewed. You have not quite managed it, I am afraid. Look at lines 6‑14 where you have incorrectly indented part and correctly indented a different part. Look at lines 191‑192, which are both too far to the right. You have changed to BSD indentation almost everywhere but left a little K&R indentation in line 199. You may think I am being too fussy about indentation, but it is an importatnt tool for you to see where you are in the code and it is you who will fail to indentify the errors because the indentation is not consistent.Olivier LeMaître wrote:
Claude Moore wrote:Carefully have a look at your equals() methods. What may be wrong with them ?
Honestly, I have no idea. firstly, . . .Campbell Ritchie wrote:Start by getting rid of the long lines . . .
Sorry, I've edited the original post.
All things are lawful, but not all things are profitable.
Olivier LeMaître wrote:deleted the equals() method
Olivier LeMaître wrote:changed List initialization (Not sure what the benefit is)
Olivier LeMaître wrote:I do use the counter variable in the hirePeople() method
Olivier LeMaître wrote:Why does this give me an "cannot find symbol" error?
You are correct! I had missed it, but because you are using the counter variable, the same remark about enhanced for loop and regular for loop applies here as well
in the Person class you initialize the instance variable name with a default value, but you immediately overwrite this value in the constructor. So the default value is useless and prevents you from making the instance variable name immutable (marking it as a final variable)
And now a more conceptual improvement. You have a list with possible persons you can hire and a list to store the hired persons. And you have lots of code to keep the lists in good shape when someone is hired or fired. You even create an additional array. This can be really simplified a lot. Assume you would add a property numberHired to the Person class. You could increment this property if a person is hired or decrement if a person is fired. No need to have hiredPeopleList anymore. And you could have a getTotalSalary() method, which simply multiplies numberHired with salary.
You have deleted the equals() method, but you are still invoking this method on line96. Do you have any clue why the program is still working as expected although you have deleted this method?
Olivier LeMaître wrote:I have no idea.
Roel De Nijs wrote:
Olivier LeMaître wrote:I have no idea.
Let's shed some light on this by giving nothing but a few pointers and let you discover the reason why :)
You deleted the equals() method, but the line invoking this method still compiled successfully. Do you know why you didn't get a compiler error on this invocation?
Roel De Nijs wrote:
You deleted the equals() method, but the line invoking this method still compiled successfully. Do you know why you didn't get a compiler error on this invocation?
Olivier LeMaître wrote:Because it is part of the java.lang package and we don't have to import it?
Olivier LeMaître wrote:I know that equals() compares two objects with each other instead of "==" which only compares the reference variables.
Roel De Nijs wrote:But the question remains: you have deleted the equals() method, so why does that statement still compile? As you have deleted the equals() method, you might expect a "can't find method" compiler error, but instead the code compiles successfully. Can you explain why? Or do you have no clue?
Olivier LeMaître wrote:Stackoverfow sais it is provided by "java.lang.Object", does that mean it is part of a set of default methods that every object gets? Or am I just talking gibberish?
Roel De Nijs wrote:And now a more conceptual improvement. You have a list with possible persons you can hire and a list to store the hired persons. And you have lots of code to keep the lists in good shape when someone is hired or fired. You even create an additional array. This can be really simplified a lot. Assume you would add a property numberHired to the Person class. You could increment this property if a person is hired or decrement if a person is fired. No need to have hiredPeopleList anymore. And you could have a getTotalSalary() method, which simply multiplies numberHired with salary.
Olivier LeMaître wrote:The equals method is just the == operator brought down to a lower level?
Olivier LeMaître wrote:But why didn't the not overwritten equals() method return false, when I compared two identical objects in my original code?
Carey Brown wrote:One problem I have is that you have a "Person" class.
I don't completely understand this question. Can you try to rephrase and/or explain it a bit better?
I think this is important. One problem I have is that you have a "Person" class. Based on your usage this seems to be more of a "JobClass" class, or just "Job"
Olivier LeMaître wrote:Does equals also return true when two objects are instances of the same class but have different instance variables? (no I guess?)
Olivier LeMaître wrote:when I ran my original code (top of the post) without overwriting the equals() method for the object, equals() (like used on line 91) always returned false, even if it was the same object.
Olivier LeMaître wrote:I wanted to ask you why the equals method does work on line 96, because I thought the variables stored in hiredPeopleList aren't refering to the same objects as those in peopleList.
Then I looked at line 79, there isn't a new object being instanciated, it is the same object, just an other reference variable in an other ArrayList
Roel De Nijs wrote:
Exactly! You are spot-on!
Olivier LeMaître wrote:do you think it is a good idea if I post it here? Or is it better to just keep it just for my self?
I think I am going to make a heavily improved second version, do you think it is a good idea if I post it here?
All things are lawful, but not all things are profitable.
Roel De Nijs wrote:
Olivier LeMaître wrote:changed List initialization (Not sure what the benefit is)
One of the OOP design principles is program to an interface (supertype), not an implementation. So in your code there is very little benefit (or even none), but it is a best practice to always program to an interface (supertype) and not an implementation.
All things are lawful, but not all things are profitable.
...calling non-final methods during instance initialization can cause problems.
Constructors must not invoke overridable methods, directly or indirectly.
Knute Snortum wrote:It looks really good! I've had to get very picky to find anything.
When you continue a statement on a second line, you should indent two tabs like so
If you wanted to go farther with this, how about separating the classes into different files, setting a package, setting class and method access modifiers, documenting public methods and classes with Javadocs.
Daniel Cox wrote:I noticed that the constructor in class Staff calls a non-final method. The Java™ Tutorials warn against this because during initialization of a subclass instance, the constructor may indirectly access instance fields in the subclass instance before they have been initialized.
Did you see how Paul cut 87% off of his electric heat bill with 82 watts of micro heaters? |