Does using a lambda make this code more DRY?

Question

I started out with a function having code duplication like this:

    private static void GetAndSaveResources(string url)
    {
            ...

            if (url.Contains("[" + "access_id" + "]"))
            {
                RestCall.UriVariables.Add("access_id", 1);
            }

            if (url.Contains("[" + "affiliate_status" + "]"))
            {
                RestCall.UriVariables.Add("affiliate_status", "approved");
            }

            ...
    }

Then, because I had copy/pasted to make that code, I started trying to eliminate that duplication.

First I extracted a method:

    private static void ConfigureUriVariable(string url, string variable, object value)
    {
        if (url.Contains("[" + variable + "]"))
        {
            RestCall.UriVariables.Add(variable, value);
        }
    }

However, while that eliminated the duplication it also created a duplication of string url accross two functions so I futher reduced it to:

        Action<string,object> setUriVariable = (variable, value) =>
        {
            if (url.Contains("[" + variable + "]"))
            {
                RestCall.UriVariables.Add(variable, value);
            }
        };
        setUriVariable("access_id", 1);
        setUriVariable("affiliate_status", "approved");

However, I would have been confused by that code before I learned about lamdas so I wonder, was the last step a step in the wrong direction?

Answer 1

First, yes, I do think your solution with a lambda is a good choice. But it's worth to also consider other options:

1. Translate the closure into an object. That would mean creating a class, with a constructor that takes url (and possibly RestCall, if it's an instance property) and stores it into a filed. It also has a method that actually does the work. Basically, this is the same as you lambda, only much more code. I think this is probably not a good solution in this case.

2. Go the other way around: create a Dictionary<string, object> (or Tuple<string, object>[] if order is important for you; or an array of anonymous type) with your parameters and then iterate over that:

   var items = new Dictionary<string, object>
   {
       { "access_id", 1 },
       { "affiliate_status", "approved" }
   };
   
   foreach (var item in items)
   {
       if (url.Contains("[" + item.Key + "]"))
       {
           RestCall.UriVariables.Add(item.Key, item Value);
       }
   };
   

Answer 2

It's not really a problem, and I like to gently prod newcomers into learning features like lambdas.

That being said, I don't think that your url duplication int the first case is anti-dry, its a perfectly acceptable method extraction and it's private. Within the scope of private methods you can do pretty much whatever you like and it's not going to offend anyone too much.

Another possibility that you might consider (though it would be a greater scope) would be to extension method the url so you can do url.ConfigureUriVariable("foo", "bar"). The above would be a bit clearer if instead of a string you used a dedicated Url class. I recommend the built in Uri or make your own.

But really, I think you're all good.

BTW - the lambda is still compiled to a delegate so the generated CL is pretty similar using either of your techniques.

Answer 3

I don't think this makes your code any more "DRY", because you still have one function. It's just a matter of the scope the function exists in. If you only need this function here, then I don't think a lambda is a step in the wrong direction as it could make the rest of your code cleaner.