greuelpirat / DeepCopy

Inject deep copy constructors into C# types
MIT License
13 stars 3 forks source link

Allow to use DeepCopy method without parameters #22

Closed michalciolek closed 2 years ago

michalciolek commented 2 years ago

It is possible to add a feature to generate DeepCopy for method without parameter and static, for example:

public class TestClass
{
   public int ID {get; set;}

   [DeepCopyExtension]
  public TestClass DeepCopy() => this;
}

Now I get the exception DeepCopy() must have one parameter with the same type of return type.

If I add the parameter to bypass the exception:

public class TestClass
{
   public int ID {get; set;}

   [DeepCopyExtension]
  public TestClass DeepCopy(TestClass source) => source;
}

after compile I get:

public TestClass DeepCopy(TestClass source) => (this != null) ? new TestClass(this) : null;

Which works for me. Only the parameter is unnecessary.

greuelpirat commented 2 years ago

In principle it is possible to do this. Here are two points:

First: [DeepCopyExtension] is not designed to be a member method. Instead it was intended to be an external static extension method. That is the reason for its name. It seems I forgot to check for the static modifier. I am pretty suprised that it worked, altough there are obvious flaws in the generated code like this != null :-)

Second: In my experience it is a bad practice to have deepcopy/clone-methods, because of inheritance:

class A {  public A DeepCopy() {...} }
class B : A { }

var instance = new B()
var copy = instance.DeepCopy()

That leeds to the believe that DeepCopy returns an instance of B which will never do.

So to make this work, the method must be virtual and all derivations must implement this as well. or to be sure the class should be marked final.

I will have a deeper look at this.

greuelpirat commented 2 years ago

To be clear, the way I designed this library is to use a copy-constructor:

class A {
    public A() {}
    [InjectDeepCopy] public A(A source) {}
}

var copy = new A(original);
michalciolek commented 2 years ago

Thank you for the quick reply.

Unfortunately, I didn't understand your library. I was following the example in README.md, which only describes [DeepCopyExtension]. I didn't look at the test code to find [InjectDeepCopy]. You definitely need to update your README and add above example.

As for the deepcopy/clone-methods method, you are right!

PS. By the way, you can update the nuget references to the newer version of Fody.

greuelpirat commented 2 years ago

I updated the readme and published v1.0.15 with updated Fody reference. I hope this is fine to you.