Domanda

I am trying to port some code from Delphi to C# and I have found a construction which I cannot implement in a reasonable manner, while complying with .NET Framework design guidelines (which I address at the end of my question).

Obviously C#, Java, C++ (and many other languages) provide meanings of method/constructor overloading, but Delphi constructors can additionally have multiple names. This way it's possible to write code which directly represents the intent:

var
  Data, ParI, ParD, Locl: TDataElement;
begin
  Data := TDataElement.Create('Element');
  ParI := TDataElement.CreateParam('IntElement', 22);
  ParD := TDataElement.CreateParam('DblElement', 3.14);
  Locl := TDataElement.CreateLocal('LocalElement');
  // ... use the above objects ...
end;

Simplified code below:

unit DataManager;

interface

TDataElement = class
  FName: string;
  FPersistent: Boolean;
public
  constructor Create(AName: string);
  constructor CreateParam(AName: string; DefaultInt: Integer); overload;
  constructor CreateParam(AName: string; DefaultDouble: Double); overload;
  constructor CreateLocal(AName: string);
  property Name: string read FName;;
  property Persistent: Boolean read FPersistent;
end;

implementation

constructor TDataElement.Create(AName: string);
begin
  FName := AName;
  FPersistent := True;
  // ... other initialization ...
end;

constructor TDataElement.CreateParam(AName: string; DefaultDouble: Double);
begin
  Create(AName); 
  // ... use DefaultInt ...
end;

constructor TDataElement.CreateParam(AName: string; DefaultInt: Integer);
begin
  Create(AName); 
  // ... use DefaultDouble...
end;

constructor TDataElement.CreateLocal(AName: string);
begin
  Create(AName); 
  FPersistent := False;
  // ... other code for local (non-persistent) elements ...
end;

Specifically in C# constructor must have the same name as the class, so first I have tried to differentiate behavior with enumeration. Alas, I've stumbled upon several problems:

  • first parameter in each constructor is the same type (ElementKind)
  • constructors are not easily recognizable like in Delphi (Create vs. CreateParam vs. CreateLocal)
  • extra care is required in subclasses of DataElement
  • possibility of mistakes, like specifying ElementKind.DoubleParam and passing integer value
  • an extra bool parameter is required to handle local elements

First attempt below:

public enum ElementKind
{
    Regular, IntParam, DoubleParam, Local
}

public class DataElement
{
    private string FName;
    public string Name { get { return FName; } }

    private bool FPersistent;
    public bool Persistent { get { return FPersistent; } }

    public DataElement(ElementKind kind, string name)
    {
        FName = name;
        // ugly switch :-(
        switch (kind)
        {
            case ElementKind.Regular:
            case ElementKind.IntParam:
            case ElementKind.DoubleParam:
                FPersistent = true;
                break;
            case ElementKind.Local:
                FPersistent = false;
                break;
        }
        // ... other initialization ...
    }

    public DataElement(ElementKind kind, string name, int defaultInt)
        : this(kind, name)
    {
        // ... use defaultInt ...
    }

    public DataElement(ElementKind kind, string name, double defaultDouble)
        : this(kind, name)
    {
        // ... use defaultDouble ...
    }

    // Redundant "bool local" parameter :-(
    public DataElement(ElementKind kind, string name, bool local)
        : this(kind, name)
    {
        // What to do when "local" is false ???

        // ... other code for local (non-persistent) elements ...
    }
}

public class Program
{
    public void Run()
    {
        DataElement data = new DataElement(ElementKind.Regular, "Element");
        DataElement parI = new DataElement(ElementKind.IntParam, "IntElement", 22);
        DataElement parD = new DataElement(ElementKind.DoubleParam, "DblElement", 3.14);
        DataElement locl = new DataElement(ElementKind.Local, "LocalElement");
    }
}

Then I have tried more object-oriented way to differentiate constructors by types, while keeping the same initialization code in Run() method:

public class ElementKind
{
    public class RegularElement
    {
        internal RegularElement() { /* disallow direct creation */ }
    }
    public class IntParamElement
    {
        internal IntParamElement() { /* disallow direct creation */ }
    }
    public class DoubleParamElement
    {
        internal DoubleParamElement() { /* disallow direct creation */ }
    }
    public class LocalElement
    {
        internal LocalElement() { /* disallow direct creation */ }
    }

    public static readonly ElementKind.RegularElement Regular = new RegularElement();
    public static readonly ElementKind.IntParamElement IntParam = new IntParamElement();
    public static readonly ElementKind.DoubleParamElement DoubleParam = new DoubleParamElement();
    public static readonly ElementKind.LocalElement Local = new LocalElement();
}

public class DataElement
{
    private string FName;
    public string Name { get { return FName; } }

    private bool FPersistent;
    public bool Persistent { get { return FPersistent; } }

    protected DataElement(string name)
    {
        FName = name;
        // ... other initialization ...
    }

    public DataElement(ElementKind.RegularElement kind, string name)
        : this(name)
    {
        FPersistent = true;
    }

    public DataElement(ElementKind.IntParamElement kind, string name, int defaultInt)
        : this(name)
    {
        FPersistent = true;
        // ... use defaultInt ...
    }

    public DataElement(ElementKind.DoubleParamElement kind, string name, double defaultDouble)
        : this(name)
    {
        FPersistent = true;
        // ... use defaultDouble ...
    }

    public DataElement(ElementKind.LocalElement kind, string name)
        : this(name)
    {
        FPersistent = false;

        // ... other code for "local" elements ...
    }
}

public class Program
{
    public void Run()
    {
        DataElement data = new DataElement(ElementKind.Regular, "Element");
        DataElement parI = new DataElement(ElementKind.IntParam, "IntElement", 22);
        DataElement parD = new DataElement(ElementKind.DoubleParam, "DblElement", 3.14);
        DataElement locl = new DataElement(ElementKind.Local, "LocalElement");
    }
}

Everything compiles and works very well. So what is my problem here? .NET Framework design guidelines, and a tool named Microsoft FxCop. After running the last code through this tool I got multiple breaking problems (see below).

And the question is: how to design my classes to comply with .NET design guidelines and best practices?

Breaking - Certainty 90% - Nested types should not be visible - ElementKind+RegularElement Breaking - Certainty 90% - Nested types should not be visible - ElementKind+IntParamElement Breaking - Certainty 90% - Nested types should not be visible - ElementKind+DoubleParamElement Breaking - Certainty 90% - Nested types should not be visible - ElementKind+LocalElement

Breaking - Certainty 90% - Static holder types should not have constructors - ElementKind

Breaking - Certainty 75% - Identifiers should not contain type names - DataElement.#.ctor(ElementKind+IntParamElement,System.String,System.Int32)

Breaking - Certainty 75% - Identifiers should not contain type names - DataElement.#.ctor(ElementKind+DoubleParamElement,System.String,System.Double)

Non Breaking - Certainty 25% - Do not declare read only mutable reference types - ElementKind.#Regular

Non Breaking - Certainty 25% - Do not declare read only mutable reference types - ElementKind.#IntParam

Non Breaking - Certainty 25% - Do not declare read only mutable reference types - ElementKind.#DoubleParam

Non Breaking - Certainty 25% - Do not declare read only mutable reference types - ElementKind.#Local

È stato utile?

Soluzione

For starters, I would replace your nested "ElementKind" class with an enum:

public enum ElementKind
{
    RegularElement,
    IntParamElement,
    DoubleParamElement,
    LocalElement
}

In addition, I don't think your Delphi code needs to be mapped to a constructor. You would probably be better off using static factory methods that return a DataElement. For example:

public static DataElement Create(string name)
{
    return new DataElement(ElementKind.Regular, name);
}

public static DataElement CreateParam(string name, int defaultInt);
{
    return new DataElement(ElementKind.IntParam, name);
    // ... use defaultInt ... 
}

// similar to above
public static DataElement CreateParam(string name, double defaultDouble); 
public static DataElement CreateLocal(string name);

Since you're using the factory functions to create your DataElement objects, you should make the constructors private.

You would then update your Run() function to use these as follows:

public void Run() 
{ 
    DataElement data = DataElement.Create("Element");
    DataElement parI = DataElement.CreateParam("IntElement", 22);
    DataElement parD = DataElement.CreateParam("DblElement", 3.14); 
    DataElement locl = DataElement.CreateLocal("LocalElement"); 
} 

Update: I included the suggested changes to your Run() function and corrected the basic Create() factory method (I believe it's supposed to return a "Regular" DataElement).

Altri suggerimenti

You can have static factory methods for that:

public class TDataElement {
    private TDataElement(){}

    public static TDataElement Create(string name ) {  
        return new TDataElement {
                                   FName = name,
                                   FPersistent = true
                                   // ... other initialization ...
                                };
    }

    public static TDataElement CreateParam(string name, double defaultDouble){
         var element = Create(name);
         // ... use DefaultInt ...
         return element;
    }
//...    
}

You can use it in this way:

Data = TDataElement.Create("Element");
ParI = TDataElement.CreateParam("IntElement", 22);

This is a well known practice. For instance, you have a factory method in the class Imageof .NET framework: var image = Image.FromFile("test.jpg");

I think, this Delphi code is the good candidate to be generic in C#:

class DataElement
{
  public string Name { get; set; }
  public bool Persistent { get; set; }

  public DataElement(/* ctor params */)
  {
  }
}

class DataElement<T> : DataElement
{
  public DataElement(string name, T defaultValue /* other ctor params */)
    : base(/* base ctor params */)
  {
  }
}

class IntElement : DataElement<int> {}
class DoubleElement : DataElement<double> {}

More details about TDataElement would be useful.

Finally I came up with following generic implementation, thanks to the idea of @JonSenchyna and suggestion of @Dennis.

As a bonus feature I added IElementFactory interface to the mix, to expresses a requirement for implementing subclasses to have all InitXyz() methods - which is absent in original Delphi code.

The only thing I don't like in this design are the requirements to make default constructors of DataElement() and ByteElement() public (which come from new() constraint), as well as InitXyz() methods (because they are part of public IElementFactory interface).

public enum ElementKind
{
    RegularElement,
    IntParamElement,
    DoubleParamElement,
    LocalElement
}

public interface IElementFactory<TElement>
{
    void InitElement(string name);
    void InitParam(string name, int defaultValue);
    void InitParam(string name, double defaultValue);
    void InitLocal(string name);
}

public class DataElement<TElement>
    where TElement : class, IElementFactory<TElement>, new()
{
    private ElementKind FKind;
    public ElementKind Kind { get { return FKind; } }

    private string FName;
    public string Name { get { return FName; } }

    private int FDefaultInt;
    protected int DefaultInt
    {
        get { return FDefaultInt; }
        set { FDefaultInt = value; }
    }

    private double FDefaultDouble;
    protected double DefaultDouble
    {
        get { return FDefaultDouble; }
        set { FDefaultDouble = value; }
    }

    protected DataElement()
    {
    }

    public virtual void InitElement(ElementKind kind, string name)
    {
        FKind = kind;
        FName = name;
    }

    public static TElement Create(string name)
    {
        TElement obj = new TElement();
        obj.InitElement(name);
        return obj;
    }

    public static TElement CreateParam(string name, int defaultValue)
    {
        TElement obj = new TElement();
        obj.InitParam(name, defaultValue);
        return obj;
    }

    public static TElement CreateParam(string name, double defaultValue)
    {
        TElement obj = new TElement();
        obj.InitParam(name, defaultValue);
        return obj;
    }

    public static TElement CreateLocal(string name)
    {
        TElement obj = new TElement();
        obj.InitLocal(name);
        return obj;
    }
}

public class ByteElement : DataElement<ByteElement>, IElementFactory<ByteElement>
{
    public ByteElement()
    {
    }

    public void InitElement(string name)
    {
        base.InitElement(ElementKind.RegularElement, name);
    }

    public void InitParam(string name, int defaultValue)
    {
        base.InitElement(ElementKind.IntParamElement, name);
        // Range checking
        if ((defaultValue >= Byte.MinValue) && (defaultValue <= Byte.MaxValue))
        {
            DefaultInt = defaultValue;
        }
    }

    public void InitParam(string name, double defaultValue)
    {
        base.InitElement(ElementKind.DoubleParamElement, name);
        // Range checking
        if ((defaultValue >= Byte.MinValue) && (defaultValue <= Byte.MaxValue))
        {
            DefaultDouble = defaultValue;
        }
    }

    public void InitLocal(string name)
    {
        base.InitElement(ElementKind.LocalElement, name);
    }

}

Look how beautiful, well-intentioned and analogous to Delphi code (!) the refactored Program.Run() method is:

public class Program
{
    public static void Run()
    {
        ByteElement data = ByteElement.Create("Element");
        ByteElement parI = ByteElement.CreateParam("IntElement", 22);
        ByteElement parD = ByteElement.CreateParam("DblElement", 3.14);
        ByteElement locl = ByteElement.CreateLocal("LocalElement");
    }
}

And now the FxCop complains only about four factory methods, but I think I can live with that:

Breaking - Certainty 95% - Do not declare static members on generic types - DataElement`1.#Create(System.String)

Breaking - Certainty 95% - Do not declare static members on generic types - DataElement`1.#CreateParam(System.String,System.Int32)

Breaking - Certainty 95% - Do not declare static members on generic types - DataElement`1.#CreateParam(System.String,System.Double)

Breaking - Certainty 95% - Do not declare static members on generic types - DataElement`1.#CreateLocal(System.String)

Autorizzato sotto: CC-BY-SA insieme a attribuzione
Non affiliato a StackOverflow
scroll top