Good way to clone an object

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)
  : EntityBase(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?

You're not listing your base class, but because of the override and the method's signature in the derived type, I'm assuming something like this:

public abstract class EntityBase : ICloneable
{
   //...
   public abstract object Clone();
}

Here's what MSDN says about ICloneable (emphasis mine):


An implementation of Clone can perform either a deep copy or a shallow
copy. In a deep copy, all objects are duplicated; in a shallow copy,
only the top-level objects are duplicated and the lower levels contain
references. Because callers of Clone cannot depend on the method
performing a predictable cloning operation, we recommend that
ICloneable not be implemented in public APIs.

From that perspective, it's better to consider that "cloning" operation as nothing more than construction of a type - passing this to a constructor, and returning the created instance, sounds like a better plan.

Now, given that a FMSUser can be constructed from another instance, I don't really see a need for the Clone method to even exist at all. On top of it all, it returns an object, and you should avoid exposing System.Object in your public interfaces.

Consider this:

var clone = new FMSUser(existing); // "clone" is FMSUser.

Versus this:

var clone = existing.Clone(); // "clone" is System.Object, requires a cast to get a FMSUser.

Side note:

var CloneUser = base.Clone() as FMSUser;

CloneUser should be named cloneUser, or simply clone. By using PascalCase, you're breaking the naming convention for locals and parameters, which should be camelCase.

StackExchange Code Review Q#49739, answer score: 5