开发者

How to refactor this?

I was trying to refactor this

class AClass
{
     string Property1 { get; set; }
     string Property2 { get; set; }
     string Property3 { get; set; }

     void AMethod(AClass other)
     {
         if(String.IsNullOrEmpty(this.Property1))
         {
              this.Property1 = other.Property1;
         }

         if(String.IsNullOrEmpty(this.Property2))
         {
              this.Property2 = other.Property2;
         }

         if(String.IsNullOrEmpty(this.Property3))
         {
              this.Property3 = other.Property3;
         }
     }
 }

And the only thing I could come up was

    private string GetFirstNotNullOrEmpty(string first, string second)
    {
        if (String.IsNullOrEmpty(first))
        {
            return second;
        }

        return first;
    }

and

    this开发者_高级运维.Property1 = GetFirstNotNullOrEmpty(this.Property1, other.Property1);

Which is not exactly equivalent, but will do the job. Is there a better way to refactor this?


If you are going to do this for the N string properties of that class, you should implement that using Reflection.

Update

It's all about "teh codez", right? Here it goes:

class SomeClass
{
    public string Property0 { get; set; }
    public string Property1 { get; set; }
    public string Property2 { get; set; }
    public string Property3 { get; set; }
    public string Property4 { get; set; }
    public string Property5 { get; set; }
    public string Property6 { get; set; }
    public string Property7 { get; set; }
    public string Property8 { get; set; }
    public string Property9 { get; set; }

    public override string ToString()
    {
        //just to print out all properties and values
        foreach (PropertyInfo prop in typeof(SomeClass).GetProperties())
        {
            Console.WriteLine(prop.Name + "," + prop.PropertyType + " = " + prop.GetValue(this, null));
        }
        return base.ToString();
    }

    public void CopyStringPropertiesIfEmptyFrom(SomeClass SourceInstance)
    {
        foreach (PropertyInfo prop in typeof(SomeClass).GetProperties())
        {
            if (prop.PropertyType == typeof(System.String) && String.IsNullOrEmpty((string)prop.GetValue(this, null)))
            {
                prop.SetValue(this, prop.GetValue(SourceInstance, null), null);
            }
        }
    }

}


Instead of using a method you could collapse the ifs into ternary operators:

this.Property1 = String.IsNullOrEmpty(this.Property1)? other.Property1 : this.Property1;


implement the check in the properties themselves.

public class AClass
{
    string Property1 
    { 
        get { return _Property1; }
        set
        {
            if (String.IsNullOrEmpty(_Property1))
            {
                _Property1 = value
            }
        }
    }
    private string _Property1;


    void AMethod(AClass other)
    {
        this.Property1 = other.Property1;// Property can only be set once.
    }

}


I'm not a fan of using Reflection when I can avoid it, so I actually like your suggested option in the question, but mixed with Tesserex's answer slightly:

private string GetFirstNotNullOrEmpty(string first, string second)
{
    return String.IsNullOrEmpty(first)) ? second : first;
}


I think the best solution would be something like

private void SetFirstNotNullOrEmpty(string first, string second, Action<T> setter)
{
    if (String.IsNullOrEmpty(first))
    {
        setter(second);
    }
}

and it would be called like this:

this.Property1 = GetFirstNotNullOrEmpty(this.Property1, other.Property1, i => this.Property1 = i);

This would be nicer if those weren't C# properties. With public fields I could pass the reference and have both the getter and the setter in a single parameter.


The first thing that needs to be refactored here is unintuitive names such as Property1 and AClass. Use meaningful names for class and property names so that they reflect the intent clearly.

Probably, the OP wants us to focus on the problem in hand and not on this aspect.

0

上一篇:

下一篇:

精彩评论

暂无评论...
验证码 换一张
取 消

最新问答

问答排行榜