3
public class Foo{
    private String a;
    private int b;

    public Foo(Foo foo){
        this.a = foo.a;
        this.b = foo.b;
    }
}

Hi everyone.

I had this code in a small part I did at work.. My colleague saw this and gave me the "you-don't-deserve-to-breathe" look and went out for about 30 minutes to calm down. (I'm a fresh grad)

I've been trying to find out what is the shameful mistake I've made.. It was not successful.

Will somebody please explain exactly why is it a bad practice (or idiotic) to have this?

The reason I did this was that the class had many params, and I didn't want to have like 3 lines of parameters being passed (using primitive parameters) every time I needed to initialize this object.

And, FYI this object is (as we call it in work) a transaction object which is initialized whenever we need to pass around an entity class (it used instead of an entity class).

I do have a default constructor as well.

Thanks!

Riccardo T.
  • 8,907
  • 5
  • 38
  • 78
Amin
  • 578
  • 1
  • 6
  • 18
  • 6
    it is called _copy constructor_ and i do not see anything inherently shameful about it; it is much better than relying on _clone_; http://stackoverflow.com/questions/2427883/clone-vs-copy-constructor-which-is-recommended-in-java – guido Sep 22 '14 at 10:49
  • How would you use this constructor? `new Foo(new Foo(new Foo(...)))`? – stuXnet Sep 22 '14 at 10:49
  • 1
    You need at least one default constructor or a factory method or something like that. A constructor with the class itself is quite useful if you want to make a clone, as guido said. – stuXnet Sep 22 '14 at 10:51
  • You have presumably removed at least one other constructor here? Which is prompting some of the answers I guess you're not looking for (ie. [this](http://stackoverflow.com/a/25972296/432913), and [this](http://stackoverflow.com/a/25972288/432913).) – will Sep 22 '14 at 10:53
  • @will, yes I forgot to put the default constructor, but I do have a default constructor in the real code. I didn't edit my question, as then some answers would have to edit as well. – Amin Sep 22 '14 at 11:16
  • @user1807954 although, the article starts to explains `clone` as if it's a horrible nightmare to use, but then doesn't mention anything which sounds like it would make it difficult to use. – will Sep 22 '14 at 11:24

3 Answers3

6

There is nothing wrong with your code. Actually what you did has a name: copy constructor And it is very convenient way to make o copy of another object. (assuming you have some other way to create instance of it besides this constructor )

Kamil.H
  • 458
  • 2
  • 8
  • Thanks you very much for your answer.. I was just missing the right vocabs for it.. now that I search copy constructor for Java, I found this link which I think is the best way to make a copy constructor. What do you think? http://www.javapractices.com/topic/TopicAction.do?Id=12 – Amin Sep 22 '14 at 10:55
  • There might be nothing wrong with his code. The exception is when the original constructor does some other stuff other than initializing fields: when using such a copy constructor, he might miss a part of the initialization. – Riccardo T. Sep 22 '14 at 11:05
2

You have only 1 constructor, so in order to create an object of class Foo, you need to pass to the constructor a Foo and in order to create that Foo you will need another Foo and it will go on and on . The code can make much more sense if for e.g. you will have a default constructor in the class and then you constructor

public Foo(Foo foo){
    this.a = foo.a;
    this.b = foo.b;
}

will act more like a Copy constructor in

sol4me
  • 15,233
  • 5
  • 34
  • 34
1

Constructor get calls when you want to create an instance using new keyword. All the classes have a default constructor which is like classname(){}

In your example you have a separate constructor. Therefore the default constructor is no longer exist for your class. The only constructor that you have now is

public Foo(Foo foo){}

According to the your source you must pass an instance of Foo to create an new instance of Foo class. So you can try

Foo fooObj = new Foo(new Foo(new Foo(.......))

As you can see that this will never get end.

You can pass null as the parameter to constructor

Foo fooObj = new Foo(null)

But since you are using the instance variables of reference Foo object to initiate the values of new Foo object it will not work either.

What you should do is add the default constructor to your class.

public class Foo{
    private String a;
    private int b;

    public Foo(){}

    public Foo(Foo foo){
        this.a = foo.a;
        this.b = foo.b;
    }
}

Then you can create instances of Foo class as follows

Foo f1 = new Foo();
f1.setA("ABC"); // create getters and setters
f1.setB(12);

Foo f2 = new Foo(f1);
  • What you said makes sense.. it was my bad to not mention the default constructor as I do have a default constructor in the real code. And please read what I commented on @user1427708's answer – Amin Sep 22 '14 at 11:01