• Post Reply Bookmark Topic Watch Topic
  • New Topic

Passing Object As Parameter  RSS feed

 
Ranch Hand
Posts: 119
Eclipse IDE Mac VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have an object that has an instance of another object class as its parameter :
CombinationLock oneHundred = new CombinationLock(28,17,39);
Locker Mickey = new Locker(100, "Mickey", 3, oneHundred);

This is for a locker, which has a combination assigned to the student. Within the locker class I have the following constructor:


combo is the private CombinationLocker object I created within the Locker class. Do I need to pass the combo object on to the CombinationLock class?
For reason, I do not comprehend, the combination password from the main class is not passing through to the CombinationLock class, and the combination values are all zero. If you like I can post more of the code or message someone, but I would appreciate any help. Thank you
 
Derek Smiths
Ranch Hand
Posts: 119
Eclipse IDE Mac VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have figured out the problem -- well actually, it wasn't in the coding, I was reseting the values of the combination before I compared them to the combination. Anyways, if you are able to delete this post, by all means.
 
Marshal
Posts: 56600
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
We don't delete posts which might be helpful to others in future.
Why on earth have you called the lock object oneHundred? That is a very confusing name.
 
Campbell Ritchie
Marshal
Posts: 56600
172
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Derek Smiths wrote: . . .
combo is the private CombinationLocker object I created within the Locker class.
No, it isn't. You haven't created any objects there, simply copied the reference to combo. Do you mean combination locker or combination lock?
Is the combination lock object immutable? If not, you are in a dangerous situation there. What if somebody resets the code in your lock?
oneHundred.setCode(12, 34, 56); ?
Now you can't open your locker any more. Either make the lock object immutable or take a defensive copy of it.
Do I need to pass the combo object on to the CombinationLock class?
Don't pass things back and forth in the constructor. A constructor is for initialising the onject it is in, and should not be used for anything else. If you start passing things back and forth there is a risk of letting a reference to the locker object escape before it is completely initialised and that can lead to subtle and potentially dangerous errors elsewhere.

For reason, I do not comprehend, the combination password from the main class is not passing through to the CombinationLock class, and the combination values are all zero.
Does your lock class have a no‑arguments constructor? If so, delete that constructor. If you make the object immutable, the compiler may insist you remove that constructor.
If you like I can post more of the code or message someone, but I would appreciate any help. Thank you
Please post the code here and don't send messages.
 
Derek Smiths
Ranch Hand
Posts: 119
Eclipse IDE Mac VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:We don't delete posts which might be helpful to others in future.
Why on earth have you called the lock object oneHundred? That is a very confusing name.


Well, because the lock combination is associated with locker 100. It makes sense to me.
 
Derek Smiths
Ranch Hand
Posts: 119
Eclipse IDE Mac VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you for your remarks Campbell. My final code works out the way I would like it to, but I'm not sure about security measures, as you mentioned. The combination lock object is immutable, as far as I know.
Now that you have bought it to my attention, I can see why it would be dangerous to pass objects within initializers...unfortunately, at the time when I was looking at an example online, a professor had done just that in his coding. That is why I thought that was the only way to implement the requirements of the assignment.

I have attached my final code and would greatly appreciate your thoughts and criticism of the program.






 
Bartender
Posts: 1840
10
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Just as a question, what happens you you enter the combination 39, 17, 28 for Mickey's locker?

Some basic reactions:
You have an array for 300 lockers indexed by locker number, but are only using two entries out of those 300.
I would use a Map rather than an array if it was sparsely populated like this. Not wrong, just strikes me as inefficient :-)

Naming of your CombinationLock objects has already been brought up. You said it refers to the Locker number, and it makes sense to you.
My argument would be that it has to make sense to the next person coming along to maintain your code (or in this case, mark your code). Using the work "lock" in a variable of type combination lock would make sense to me.
i.e. lock100 or even lockForLocker100, and would actually indicate what this variable represents :-)
Similar for "Mickey" and "Donald" which are lockers, and not actually people, though you wouldn't know it from looking at the variable name.


The CombinationLock class - I'm not certain of the use of the leftTurn/RightTurn methods when you have a "open locker" method. Is there any reason they are necessary?
And also as I mentioned above, you can open the locker with the "wrong" combination.
Theoretically it should only open if you call left, right, left IN THE RIGHT ORDER.

The locker constructor takes a "lock" object, but the "set combination" method takes three numbers. Theoretically setCombination should call lock.setCombination(...), or should be given a Lock object like it was in the constructor. Its a matter of consistency




 
Derek Smiths
Ranch Hand
Posts: 119
Eclipse IDE Mac VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Stefan Evans wrote:Just as a question, what happens you you enter the combination 39, 17, 28 for Mickey's locker?

Some basic reactions:
You have an array for 300 lockers indexed by locker number, but are only using two entries out of those 300.
I would use a Map rather than an array if it was sparsely populated like this. Not wrong, just strikes me as inefficient :-)

Naming of your CombinationLock objects has already been brought up. You said it refers to the Locker number, and it makes sense to you.
My argument would be that it has to make sense to the next person coming along to maintain your code (or in this case, mark your code). Using the work "lock" in a variable of type combination lock would make sense to me.
i.e. lock100 or even lockForLocker100, and would actually indicate what this variable represents :-)
Similar for "Mickey" and "Donald" which are lockers, and not actually people, though you wouldn't know it from looking at the variable name.


The CombinationLock class - I'm not certain of the use of the leftTurn/RightTurn methods when you have a "open locker" method. Is there any reason they are necessary?
And also as I mentioned above, you can open the locker with the "wrong" combination.
Theoretically it should only open if you call left, right, left IN THE RIGHT ORDER.



Thank you Stefan. Definitely helpful to get some constructive feedback. My teacher gave me 10/10 which is good for grade but without any feedback its difficult to improve, so thank you.

I'm not familiar with Map as of yet and just decided to create an array for typical school size...don't really know why I chose 300, I guess bc it fit both the locker numbers that we were to include in the assignment. Yes, I tend to error on the conservative side when I code my variable names, and certainly see why that could be confusing to understand for others.

I wasn't really sure the purpose or intent of using the leftTurn/rightTurn methods (requirement per assignment), so I implemented them the best way I knew how at the time. I see where I should probably add some additional protection to both those methods so that the combination cannot be entered in reverse. I'm going to go back and see what changes I can make to improve on that. Aside from the whole combination order-thing, do you see any issues as far as security is concerned? I know Campbell was wondering whether my locker combination object was secure.

Again, I really appreciate the feedback.
 
Stefan Evans
Bartender
Posts: 1840
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I would probably probably have modelled the CombinationLock class a little differently.

Methods turnLeft(x) and turnRight(x) would not return any information.
The method openLock() would take no parameters, and merely TRY to open the lock. The lock would only open (return true) if you had successfully dialled in the combination already.
That means you have to store more information in your class: The "opening" combination and "the combination the person has input to date"

So the interaction would be


The only method that should return feedback (true/false) is lock.open.
That would be modelling it more like I see a physical combination lock in real life.

But maybe I'm over-complicating things :-)
 
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Stefan Evans wrote:
But maybe I'm over-complicating things :-)

No, that makes perfect sense to me and it's straightforward and simple.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The thing with the CombinationLock class is that its leftTurn and rightTurn methods are both public, which they shouldn't be since there's already a public openLock method that takes the full lock combination and calls the leftTurn and rightTurn methods in turn. The access level for leftTurn and rightTurn should be at most package private (default). I would actually try to make the CombinationLock a private class of Locker to ensure that only the Locker class can access it, just as a combination lock is often an integral part of a locker door. Then move the openLock method to the Locker class, which then calls the privately accessible leftTurn and rightTurn methods of the CombinationLock.

I know that probably sounds a lot more complicated than what you currently have but it's about limiting visibility of class members to only those classes that really need to use them. This is one way to keep your code under control and more secure.
 
Derek Smiths
Ranch Hand
Posts: 119
Eclipse IDE Mac VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm going to digest what you both have said and get back to you! :P
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It's always useful to have something to look at while you're trying to digest so here's a sample skeleton:

This is not to say that this design is ideal, it's just something to get started for playing around with some ideas.
 
Ranch Hand
Posts: 934
4
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Junilu.. We are missing teachers like you.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You're welcome!

Don't think I don't get anything out of it either -- gotta do everything I can to keep the old glob of gray matter from becoming decrepit
 
Tushar Goel
Ranch Hand
Posts: 934
4
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Pardon me for my English but i don't understand what you said..
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Means I have to do what I can to keep my brain from getting worn out from old age

Decrepit
Glob
Grey/Gray Matter
 
Tushar Goel
Ranch Hand
Posts: 934
4
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
 
It is sorta covered in the JavaRanch Style Guide.
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!