Is It More Readable??

I was just going over some code and came across some webservice methods having a lot of repetitive conditional statements checking the language - represented by the corresponding two-letter code and send by the client from JavaScript. Based on the outcome, the code was calling two different methods on the BL layer.

public List<Nation> GetCitizenship(string abbr, string lang)
{
   IList<Nation> nat = null;
   if (lang == "it")
   {
      nat = NationBL.ReadAllNationsByDescIt(abbr, false, null);
   }
   else
   {
      nat = NationBL.ReadAllNationsByDescDe(abbr, false, null);
   }

   return nat.ToList();
}

This cluttered up the whole method and moreover the same kind of duplication was present on more methods on that service class. Now, without considering the fact that the NationBL should probably be structured differently s.t. you don't have to call two different kind of methods just because of the language, I came up with the following refactoring:

private IList<TItem> GetLocalizedList<TItem>(string lang, Func<IList<TItem>> blfunctionDe, Func<IList<TItem>> blfunctionIt)
{
    IList<TItem> result;
    if (lang == "it")
    {
        result = blfunctionIt.Invoke();
    }
    else
    {
        result = blfunctionDe.Invoke();
    }

    if(result != null)
       return result.ToList<TItem>;
    else
       return null;
}

This method returns you the correct list by invoking the right delegate (passed as parameter), depending on the kind of language (lang param) it received. In this way, the check for the language is centralized in the method and could arbitrarily be adapted/substituted without having to touch code in ten different places.

The implementation of the original service method can then be changed by calling the new GetLocalizedList method:

[WebMethod]
[ScriptMethod(ResponseFormat = ResponseFormat.Json)]
public List<Nation> GetCitizenship(string abbr, string lang)
{
    return GetLocalizedList(
              lang, 
              () => NationBL.ReadAllNationsByCitizenshipIt(abbr, false, null), 
              () => NationBL.ReadAllNationsByCitizenshipDe(abbr, false, null));
}

While certainly this refactoring eliminates code duplication, I'm not sure whether it improves code readability...

---

Questions? Thoughts? Hit me up on Twitter