2

I have to clone some Entity, then I wrote this piece of code.

public override object Clone()
{
  var CloneUser = base.Clone() as FMSUser;

  CloneUser.Username = this.Username;
  CloneUser.IsEnabled = this.IsEnabled;
  CloneUser.IsNeedPasswordReset = this.IsNeedPasswordReset;
  CloneUser.LastName = this.LastName;
  CloneUser.FirstName = this.FirstName;
  CloneUser.MiddleName = this.MiddleName;
  CloneUser.DistributorID = this.DistributorID;
  CloneUser.IsLocked = this.IsLocked;

  return CloneUser;
}

But then my coworker sent me this code, saying it's better to clone this way, but can't tell me why : 

public FMSUser(FMSUser user)
{   
  this.Username = user.Username;
  this.IsEnabled = user.IsEnabled;
  this.IsNeedPasswordReset = user.IsNeedPasswordReset;
  this.LastName = user.LastName;
  this.FirstName = user.FirstName;
  this.MiddleName = user.MiddleName;
  this.DistributorID = user.DistributorID;
  this.IsLocked = user.IsLocked;
}
public override object Clone()
{
  return new FMSUser(this);
}

Can anynone explain me why the second way is better?

poudigne
  • 1,694
  • 3
  • 17
  • 40
  • 3
    You should avoid `Clone` in general. Instead provide a `DeepClone` method(or whatever name you want). You don't know how `Clone` is implemented, using deep or shallow copy. – Tim Schmelter May 14 '14 at 15:22
  • 1
    It's not better, it's just wrapped so the code can be used from parameter-based contructor. But keep in mind the second method doesn't do anything with the base class, so it looks like the base is gonna have default values instead of cloned ones. – Tarec May 14 '14 at 15:23
  • 1
    I don't think Clone is the best way to do this. You could use reflection instead and clone the properties dynamically, that way you won't have to worry about changes in the classes as long as the properties are identical(same type, name, etc) and accessible from one object to another. – stripthesoul May 14 '14 at 15:26
  • 1
    This sounds like a question for CodeReview? – Christoph Fink May 14 '14 at 15:27
  • From your code it seems like CloneUser is not a class that needs to be in your main class. Your co-worker seems like wanted to remove the confusion between CloneUser and FMSUser. – Bura Chuhadar May 14 '14 at 15:32
  • 4
    If he can't tell you why it is better, it is not better, it is just an opinion. The first method does something with the base class, the second just ignores the base class properties. Without the whole implementation, we can't tell if it makes a difference. – Peter May 14 '14 at 15:33
  • Here's a great SO discussion covering this exact topic: http://stackoverflow.com/questions/78536/deep-cloning-objects – Colby Cavin May 14 '14 at 15:46

2 Answers2

1

Clone() is ambiguous and even Microsoft recommends not implementing ICloneable

What I think is cleaner is just a ctor that takes the object
Clone in that ctor 

 FMSUser copyUser = new FMSUser(existingUser);

Then also have a method

 public FMSUser DeepClone() 
 {
    return new FMSUser(this);
 }

ICloneable Interface

Object.MemberwiseClone Method

paparazzo
  • 44,497
  • 23
  • 105
  • 176
-2

If your object is serializable.

public static T Clone<T>(T obj)
{
 using (var ms = new MemoryStream())
 {
   var formatter = new BinaryFormatter();
   formatter.Serialize(ms, obj);
   ms.Position = 0;

   return (T) formatter.Deserialize(ms);
 }
}
Ventsyslav Raikov
  • 6,882
  • 1
  • 25
  • 28