tags:

views:

302

answers:

7
class Creature {    
   private int yearOfBirth=10;

   public void setYearOfBirth(int year) {
      yearOfBirth = year;
   }

   void setYearOfBirth(Creature other) { 
      yearOfBirth = other.yearOfBirth; // is this correct it compiles fine 
   }

   int getYearOfBirth() { 
      return yearOfBirth;
   } 

   public static void main(String args[])
   {
      Creature c = new Creature();
      c.setYearOfBirth(89);

      Creature d = new Creature();
      c.setYearOfBirth(d);

      System.out.println(c.yearOfBirth);
   }
}

Is there any mistake in this code?

Is "other.yearOfBirth" wrong? My faculty says it is wrong but it works fine for me.

+5  A: 

You have to ask your faculty to explain why they think it's wrong (its perhaps a question of style, or even a misunderstanding), so you can learn from it.

Ultimately this person is going to impact your grades. This is an excellent opportunity to have a positive interaction with them. The more involved your teachers are with teaching you personally, the better your opportunity for mastering your subject will be.

If on the other hand when you're told something is wrong you go away privately and ask the general internet community, there is a risk that you'll be told you're right and you'll end up a false sense of superiority over your teachers which will be very counterproductive.

Will
You *could* ask the faculty, or you could ask the thousands of Java coders here, many of whom will tell you at great lengths what the problems are, probably to the point where you'd rather cut off your own ears than listen to them any more :-)
paxdiablo
Look on it as a learning experience. Is it wrong because it doesn't compile? Or is it wrong because although it compiles, there's a problem. Or is it wrong because it doesn't solve the problem you were asked to solve? There's plenty of production code out there that WORKS, but is still just plain wrong!
Daniel Ives
I think this is a good point, and it may extend to the business world as well. Sometimes you may be asked to do something in a way that is not how you would otherwise do it, perhaps even in a way that seems wrong ... it's good to understand as best you can why you need to do it that way, so then you know how to keep your boss/coworker/instructor pleased and also know how you'd do it if they weren't involved.
Dave DuPlantis
A: 

It's wrong because you're accessing a private member (you declared private int yearOfBirth) of another object although the class type is the same. You should use the public getter you defined instead: yearOfBirth = other.getYearOfBirth()

Miguel Ventura
Why is that wrong?
Jesper
This answer also applies to the private member access from the System.out.println() call in main.
highlycaffeinated
Style is debatable but calling this wrong is just well.. wrong :p
NickDK
No, it's wrong - you should always use your mutators and accessors to ensure the data is well-formed and validated. The mutator and accessor methods can very well perform data validation and output formatting and it's a good habit to get into to always use them.
Thomas Owens
So every implemented equals method I've seen (standard java library included) that use their own private instance field to determine equality is wrong?
NickDK
A: 

yearofBirth is a private int. Therefore the call to other.yearOfBirth would presumably fail...

Mark Mayo
No, it would not, if it's inside the same class.
Jesper
Even though it might not explicitly fail, it is still incorrect. The point of implementing getter methods is to use them.
Allyn
+7  A: 

As written, it will work, as you discovered. I suspect that there's a fundamental misunderstanding at play, though.

My psychic powers tell me that your instructor expected code more like the following:

class Creature {    
   private int yearOfBirth=10;

   public void setYearOfBirth(int year) {
      yearOfBirth = year;
   }

   public void setYearOfBirth(Creature other) { 
      yearOfBirth = other.yearOfBirth;
   }

   public int getYearOfBirth() { 
      return yearOfBirth;
   } 
}

class Program {
   public static void main(String args[]) {
      Creature c = new Creature();
      c.setYearOfBirth(89);

      Creature d = new Creature();
      c.setYearOfBirth(d);

      System.out.println(c.yearOfBirth); // This will not compile
   }
}

The misunderstanding is that you've only created one class-- your main application class. This effectively makes yearOfBirth a sort of hybrid global value that you can access from your main method. In more typical designs, Creature is a class that is completely independent of your main method. In that case, you must only access Creature through its public interface. You would not be able to access its private field directly.


(Note to any pedants out there: Yes, I know I'm simplifying.)

Greg D
Nicely analyzed. Somehow, you also show the limit of simplified code used for quick demo purpose, which works in their limited environment but are wrong in real use. Similar to these snippets omitting to close streams in finally for conciseness and so on.
PhiLho
yeah this is correct i to tried and found out the same but my question is "yearOfBirth = other.yearOfBirth "will this work in class creature ? its working fine if i compile class creature in my eclipse ID
Arunachalam
Personally, I don't consider `yearOfBirth = other.yearOfBirth` to be bad in an objective way. A class can and should understand its internals and how to safely manipulate them. The merging of the two classes into one and referencing c.yearOfBirth from your main method is something I would consider much less likely to be legit.
Greg D
Yes, `yearOfBirth = other.yearOfBirth ` does work, an OO minded instructor probably wants you to use the `other.getYearOfBirth()` method though.
rsp
A: 

No, there is no problem at all with it.

Look, it depends on the viewer opinion. But for a given context this code may be just perfect.

For some other context this may not be correct. So it depends of how is going to be used.

  • Accessing a private member directly from another instance, is correct ( not always desirable though, for instance when you're subclassing ) that's why it is private in first place. You are saying "Hey, this is mine and I know how to use it"

  • Using the default access modifier for the other two methods, says that your intention is they should not be used by other classes outside the package.

Probably the only thing I would add is to make the class final.

final class Creature

If you want to make it inheritable you probably have to review the get/set for the yearOfBirth attribute, but they way it is is perfect to me.

NOW The most important thing here, is that you understand what each part of your code does, and how it affects its behavior.

You should no code just by luck ( sorry I don't know what's the correct expression for this ) but you should know what you're doing each time you type something, and how do you intend to be used.

OscarRyz
A: 

I see two "issues," though I hesitate to call them mistakes:

  1. You're explicitly setting Creature c's age as 89, and then rewriting that age with the uninitialized default (!) of Creature d. If this is what you intend to do, then fine, but at the very least you're wasting a few cycles to set a value that you intend to throw out later.

  2. You're possibly violating the JavaBeans naming conventions.

To explain the latter point: a lot of Java libraries and frameworks (notably JSP) rely on JavaBeans to treat the object as a component. I haven't dived deeply into the actual mechanisms used, but from what I've read it relies on Introspection of the JavaBeans class to infer properties and the types of those properties. By overloading your setter setYearOfBirth() to accept both an int and a Creature, you could throw off the Introspection. (See here for a decent introduction to JavaBeans.)

This is not a big deal -- its entirely possible that you won't use this class as a JavaBean, and if you do it's trivial to refactor it so it works. But your teachers would probably prefer something a little cleaner, like the following:

class Creature {    
   private int yearOfBirth=10;

   public void setYearOfBirth(int year) {
      yearOfBirth = year;
   }

   int getYearOfBirth() { 
      return yearOfBirth;
   } 

   public static void main(String args[])
   {
      Creature c = new Creature();
      c.setYearOfBirth(89);

      Creature d = new Creature();
      c.setYearOfBirth(d.getYearOfBirth());

      System.out.println(c.getYearOfBirth());
   }
}

Now all of your access to yearOfBirth comes via the public getter methods, which helps encapsulation, and will prevent the code from breaking if your main method moves to another class. (As Greg D correctly pointed out.)

Also, this has the added benefit of making the intent of your code clear, which becomes more and more important when you start writing code for others to maintain and modify.

rtperson
+1  A: 

i detect nothing wrong.

the code works, because an instance or class can access private members of other instances of the same class. this is by design.

Salandur
The issue isn't whether or not it works -- clearly, it works. The issue is whether or not there's a better (that is to say, less fragile) way to design this class. You could code an entire web server using only one class and making all member variables public. You could even get it to work, but the resulting app would likely be so difficult to maintain that the entire thing may as well be "wrong," not functionally, but because your code adheres to the "big ball of mud" antipattern.
rtperson
A class must be able to trust itself to properly handle its own private members, even if those members belong to a different instance. This isn't inherently fragile, especially if the manipulation is read-only. If you encounter a situation where you find this questionable, you have a class that's too big and should split it up.
Greg D
"can access private members of other instances of the same class" - actually I did not know that, despite my frequent use of Java. It seems dirty somehow :-) +1 for expanding my knowledge.
paxdiablo