Make this C# code more DRY

Question

public sealed class LinkXPathRepository : ILinkXPathRepository
{
    private static readonly ILog _log = LogManager.GetLogger(typeof(LinkXPathRepository));

    internal ILink Parse(Uri endpoint)
    {
        if (endpoint == null)
        {
            throw new ArgumentNullException("endpoint");
        }
        string leftPart = endpoint.GetLeftPart(UriPartial.Authority);

        EntityDataModelContext dc = DataContext.GetDataContext();

        IList<LinkXPath> sources = null;
        HtmlDocument document = null;

        Task t1 = Task.Factory.StartNew(() => sources = dc.LinkXPaths.Where(p => leftPart.Contains(p.Host)).OrderByDescending(p => p.Priority).ToList());
        Task t2 = Task.Factory.StartNew(() => document = Utility.DownloadHtml(endpoint));
        Task.WaitAll(new[] { t1, t2 });

        if (document == null)
        {
            return null;
        }

        Link result = new Link();
        foreach (LinkXPath source in sources)
        {
            if (result.Title.NullOrEmpty())
            {
                string title = GetFragment(document, source.Title);
                result.Title = title;
            }
            if (result.Description.NullOrEmpty())
            {
                string description = GetFragment(document, source.Description);
                result.Description = description;
            }
            if (result.Article.NullOrEmpty())
            {
                string article = GetFragment(document, source.Article);
                result.Article = article;
            }
        }

        return result;
    }

    internal string GetFragment(HtmlDocument document, string xpath)
    {
        return document.DocumentNode.GetChildNodeHtml(xpath);
    }
}

I'm particularly interested in refactoring this fragment:

            if (result.Title.NullOrEmpty())
            {
                string title = GetFragment(document, source.Title);
                result.Title = title;
            }
            if (result.Description.NullOrEmpty())
            {
                string description = GetFragment(document, source.Description);
                result.Description = description;
            }
            if (result.Article.NullOrEmpty())
            {
                string article = GetFragment(document, source.Article);
                result.Article = article;
            }

EDIT: please note that result.X and source.X come from different classes, even though their names are the same.

Answer 1

I propose this:

internal ILink Parse(Uri endpoint) {
    ...
    foreach (LinkXPath source in sources) {
        UpdateIfNecessary(document, ref result.Title, () => GetFragment(document, source.Title));
        UpdateIfNecessary(document, ref result.Description, () => GetFragment(document, source.Title));
        UpdateIfNecessary(document, ref result.Article, () => GetFragment(document, source.Article));
    }
    ...
}

private static void UpdateIfNecessary(Document document, ref string field, Func<string> getValue) {
    if(!string.IsNullOrEmpty(field)) {
        return;
    }
    field = getValue();
}

UpdateIfNecessary receives a lambda so it only evaluates the new value if necessary.

The field is passed by reference, so any change to the parameter in the method is reflected in the underlying argument variable in the calling method.

Answer 2

Do something like this:

result.Title       = GetFragment(document, source.Title);
result.Description = GetFragment(document, source.Description);
result.Article     = GetFragment(document, source.Article);

private string GetFragment(Document document, string field)
{
  if(string.IsNullOrEmpty(field)) return null;

  ...
}

Answer 3

This should work, though without a working swath of source, I can't say for sure.

        foreach (var source in sources)
        {
            var sourcetype = source.GetType();
            var resulttype = result.GetType();
            const BindingFlags BindingFlags = BindingFlags.Instance | BindingFlags.Public;
            var sourceProperties = sourcetype.GetProperties(BindingFlags);
            var resultProperties = resulttype.GetProperties(BindingFlags);
            var commonproperties = from sp in sourceProperties
                join rp in resultProperties on new { sp.Name, sp.PropertyType } equals new { rp.Name, rp.PropertyType }
                where sp.CanRead && rp.CanWrite
                select new { sp, rp };

            foreach (var match in commonproperties)
            {
                if (!(match.rp.GetValue(result, null) as string).NullOrEmpty())
                {
                    continue;
                }

                var value = this.GetFragment(document, match.sp.GetValue(source, null) as string);
                match.rp.SetValue(result, value, null);
            }
        }

Answer 4

1. Sample application

Disclaimer: The code you are about to see may hurt your eyes, but I sure had fun making it so I thought I'd share it! :)

Thanks for your great question, I took the liberty to have a play with generics and Expressions and went a bit functional (after all it is Friday!), came up with a refactored solution using a form of ModelMapping.

First I'd like to demonstrate the sample program:

class Program
{

    static void Main(string[] args)
    {
        HtmlDocument doc = new HtmlDocument(); //This is a fake document
        doc.Text = "MockText";

        Link  link1 = new Link();
        Link  link2 = new Link();
        link2.Title = "NotNullOrEmpty";

        LinkX linkXpath = new LinkX
        {
            Title = "xpath/to/title",
            Article = "xpath/to/article",
            Description = "xpath/to/description"
        };

        LinkModelMapper.Map(link1, linkXpath, doc); // this class will take care of all the mapping for you. 

        Console.WriteLine("link1 -> ");
        Console.WriteLine(link1.ToString()); // your link will be populated with appropriate values

        LinkModelMapper.Map(link2, linkXpath, doc);


        Console.WriteLine("link2 -> ");  // likewise here
        Console.WriteLine(link2.ToString());

        Console.ReadKey();
    }
}

And this is the application output:

So far, so good. Seems to be like what we want on the surface. So what goes on under the hood? Well this is where it becomes a bit Funcy

What is LinkModelMapper:

public class LinkModelMapper 
{
    private static readonly ModelMapper<Link, LinkX> _modelMapper;

    static LinkModelMapper()
    {
        _modelMapper = new ModelMapper<Link, LinkX>(GetConversionAction);
        _modelMapper.RegisterMapping((link) => link.Title, (linkx, args) => GetFragment(args[0], linkx.Title));
        _modelMapper.RegisterMapping((link) => link.Description, (linkx, args) => GetFragment(args[0], linkx.Description));
        _modelMapper.RegisterMapping((link) => link.Article, (linkx, args) => GetFragment(args[0], linkx.Article));
    }

    private static Action<Link, LinkX, object[]> GetConversionAction(Func<Link, string> resultGetter, Action<Link, string> resultSetter, Func<LinkX, object[], string> sourceGetter)
    {
        return (link, linkx, objectParams) =>
            {
                if (string.IsNullOrEmpty(resultGetter(link)))
                    resultSetter(link, sourceGetter(linkx, objectParams));
            };
    }

    private static string GetFragment(Object document, string xpath)
    {
        HtmlDocument doc = document as HtmlDocument;
        return doc.SimulateRunningXPath(xpath);
    }

    public static void Map(Link result, LinkX source, params object[] args)
    {
        _modelMapper.Map(result, source, args);
    }
}

We use the ModelMapper class (explained below) to Register a conversion action and mappings from links to xpaths. A conversion action is a delegate that requires 3 parameters.

• a delegate for the resultGetter e.g. Func<Link,String> = (link) => link.Title in order to evaluate if the property is null or empty.
• a delegate for setting the result property e.g. Action<Link,string> = (link, value) => link.Title = value - This action is automatically created by the model mapper for us.
• a delegate for getting the value from the source e.g Func<LinkX,object[],string> = (link, objectParams) => GetFragment(args[0], linkx.Title) this is the value that gets passed into the result setting action delegate.

The ModelMapper class is a generic class that can be reused in case we have similar mapping operations that are needed across the code base.

public class ModelMapper<TResult, TSource>
{
    private Action<TResult, TSource, object[]> _converter;
    private Func<Func<TResult, string>, Action<TResult, string>, Func<TSource, object[], string>, Action<TResult, TSource, object[]>> _converterMappingFunc;

    public ModelMapper(Func<Func<TResult, string>, Action<TResult, string>, Func<TSource, object[], string>, Action<TResult, TSource, object[]>> converterMapping)
    {
        _converter = null;
        _converterMappingFunc = converterMapping;
    }

    public void RegisterMapping(Expression<Func<TResult, string>> resultExpression, Func<TSource,object[],string> sourceGetter)
    {
        var linkPropInfo = ((resultExpression.Body as MemberExpression).Member as System.Reflection.PropertyInfo);
        var methodInfo = linkPropInfo.GetSetMethod();
        Action<TResult, string> setValue = (l, value) => methodInfo.Invoke(l, new object[] { value });

        Func<TResult, string> resultPropertyGetter = resultExpression.Compile();
        _converter += _converterMappingFunc(resultPropertyGetter, setValue, sourceGetter);
    }

    public void Map(TResult result, TSource source, params object[] args)
    {
        _converter(result, source, args);
    }
}

Most likely I could have used custom delegate types to make it slightly more readable. But in any case this is yet another demonstration of a generic reusable approach.

2. Answering your question

Of course, while I had fun putting this sample together, I'm only trying to demonstrate that you don't need to refactor your code if that is the only place that you do this sort of mapping. In my opinion out of all the answers, the most readable is the one you already have in the question.

        if (result.Title.NullOrEmpty())
        {
            string title = GetFragment(document, source.Title);
            result.Title = title;
        }
        if (result.Description.NullOrEmpty())
        {
            string description = GetFragment(document, source.Description);
            result.Description = description;
        }
        if (result.Article.NullOrEmpty())
        {
            string article = GetFragment(document, source.Article);
            result.Article = article;
        }

My tip: Do not refactor. It is straight to the point already. While this may look repeatable on the surface, i don't think it warrants refactoring, it is the easiest to edit and for anyone to easily understand what is going on in the code.

It would only be worth considering to refactor, if you have this similar pattern in other places in your application, then a more robust solution that would have the single responsibility of performing the mapping to your result links could be an option, but as it stands I prefer your original version.

3. Code listing

For those curious enough to want to run this on their own Visual studio and pull it apart, I'm including the full code listing so you can just paste it into a new project and step through the code, it may be an interesting exercise after all!

using System;
using System.Linq.Expressions;

namespace TestConsole
{
    public class HtmlDocument
    {
        public string Text { get; set; }

        public string SimulateRunningXPath(string xPath)
        {
            return String.Format("Text={0}, applying xpath: {1}", Text, xPath);
        }
    }

    public class Link
    {
        public string Title { get; set; }
        public string Description { get; set; }
        public string Article { get; set; }

        public override string ToString()
        {
            return string.Format("Title: {0}\nDescription: {1}\nArticle: {2}", Title, Article, Description);
        }
    }

    public class LinkX
    {
        public string Title { get; set; }
        public string Description { get; set; }
        public string Article { get; set; }
    }

    public class ModelMapper<TResult, TSource>
    {
        private Action<TResult, TSource, object[]> _converter;
        private Func<Func<TResult, string>, Action<TResult, string>, Func<TSource, object[], string>, Action<TResult, TSource, object[]>> _converterMappingFunc;

        public ModelMapper(Func<Func<TResult, string>, Action<TResult, string>, Func<TSource, object[], string>, Action<TResult, TSource, object[]>> converterMapping)
        {
            _converter = null;
            _converterMappingFunc = converterMapping;
        }

        public void RegisterMapping(Expression<Func<TResult, string>> resultExpression, Func<TSource,object[],string> sourceGetter)
        {
            var linkPropInfo = ((resultExpression.Body as MemberExpression).Member as System.Reflection.PropertyInfo);
            var methodInfo = linkPropInfo.GetSetMethod();
            Action<TResult, string> setValue = (l, value) => methodInfo.Invoke(l, new object[] { value });

            Func<TResult, string> resultPropertyGetter = resultExpression.Compile();
            _converter += _converterMappingFunc(resultPropertyGetter, setValue, sourceGetter);
        }

        public void Map(TResult result, TSource source, params object[] args)
        {
            _converter(result, source, args);
        }
    }

    public class LinkModelMapper 
    {
        private static readonly ModelMapper<Link, LinkX> _modelMapper;

        static LinkModelMapper()
        {
            _modelMapper = new ModelMapper<Link, LinkX>(GetConversionAction);
            _modelMapper.RegisterMapping((link) => link.Title, (linkx, args) => GetFragment(args[0], linkx.Title));
            _modelMapper.RegisterMapping((link) => link.Description, (linkx, args) => GetFragment(args[0], linkx.Description));
            _modelMapper.RegisterMapping((link) => link.Article, (linkx, args) => GetFragment(args[0], linkx.Article));
        }

        private static Action<Link, LinkX, object[]> GetConversionAction(Func<Link, string> resultGetter, Action<Link, string> resultSetter, Func<LinkX, object[], string> sourceGetter)
        {
            return (link, linkx, objectParams) =>
                {
                    if (string.IsNullOrEmpty(resultGetter(link)))
                        resultSetter(link, sourceGetter(linkx, objectParams));
                };
        }

        private static string GetFragment(Object document, string xpath)
        {
            HtmlDocument doc = document as HtmlDocument;
            return doc.SimulateRunningXPath(xpath);
        }

        public static void Map(Link result, LinkX source, params object[] args)
        {
            _modelMapper.Map(result, source, args);
        }
    }

    class Program
    {

        static void Main(string[] args)
        {
            HtmlDocument doc = new HtmlDocument();
            doc.Text = "MockText";

            Link  link1 = new Link();
            Link  link2 = new Link();
            link2.Title = "NotNullOrEmpty";

            LinkX linkXpath = new LinkX
            {
                Title = "xpath/to/title",
                Article = "xpath/to/article",
                Description = "xpath/to/description"
            };

            LinkModelMapper.Map(link1, linkXpath, doc);

            Console.WriteLine("link1 -> ");
            Console.WriteLine(link1.ToString());

            LinkModelMapper.Map(link2, linkXpath, doc);


            Console.WriteLine("link2 -> ");
            Console.WriteLine(link2.ToString());

            Console.ReadKey();
        }
    }
}

To conclude, keep it simple (as you already have). What you have is simple and it works, so not much need to change it.