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?

回答1:

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);
            }
        }
    }

}

回答2:

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

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

回答3:

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.
    }

}

回答4:

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;
}

回答5:

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.

回答6:

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.