3
\$\begingroup\$

Is there a way to write it in more efficient and modern (Up to date design pattern) way?

 static Dictionary<string, Dictionary<string, string>> m_LUApplicationParams;
    public string LUApplicationParams(int applicationId, string GuidId, string variable)
            {
                if (m_LUApplicationParams == null)
                {
                    var blLU = new LU();
                    DataTable dt = new DataTable();
                    dt = blLU.GetLUApplicationVaribales(applicationId);


                    m_LUApplicationParams = new `Dictionary<string,Dictionary<string,string>>();`


                    for (int i = 0; i < dt.Rows.Count; i++)
                    {

                        if (!m_LUApplicationParams.ContainsKey(dt.Rows[i]["LangId"].ToString()))
                        {
                            m_LUApplicationParams.Add(dt.Rows[i]["LangId"].ToString(), new Dictionary<string, string>());
                        }


                        m_LUApplicationParams[dt.Rows[i]["LangId"].ToString()].Add(dt.Rows[i]["Variable"].ToString(), dt.Rows[i]["Content"].ToString());                                    }


                }

                return m_LUApplicationParams[GuidId].Where(y => y.Key == variable).FirstOrDefault().Value.ToString();
            }
\$\endgroup\$
2
\$\begingroup\$

Some quick notes:

  • LUApplicationParams is not a good method name. It doesn't tell me what it does, and moreover it isn't even a verb.
  • m_LUApplicationParams isn't a good name for a field or property. I'm not a fan of the "m_" naming, it contains an obscure abbreviation or acronym (LU), and it doesn't tell me what it contains, which is especially bad considering this is a Dictionary<string, Dictionary<string, string>>.
  • I'd move all of the logic inside if (m_LUApplicationParams == null) to a separate method. I even wonder whether this would be the right place for such logic (although I have done such things myself).
  • blLU doesn't tell me anything, even when it is used like var blLU = new LU();.
  • Why do you do DataTable dt = new DataTable();? It's pointless since the next line assigns a value to dt anyway.
  • "LangId" is used three times. Suppose you ever need to change it: wouldn't it be handier if it were a constant?
  • dt.Rows[i]["LangId"].ToString() is used three times: then why not assign it to a variable?
  • GuidId is a parameter and thus should be CamelCase. But is GuidId even a proper name, considering that the other code suggests it is actually an id to identify the langhuage: "LangId"?
  • What does variable represent? Even the field name isn't helping, since that is "Variable" also. I'd hate to be the next person to maintain this code since obviously a lot of information is missing from it.
  • You do .FirstOrDefault() and then immediately use the .Value of the result; but what if there is no value?
  • Also, .FirstOrDefault() suggests multiple values are possible, is that a possibility you want? And why would the first result be the correct one?
  • Why do you do .ToString() to the result of .FirstOrDefault().Value? Why do you fear that the value of a Dictionary<string, string> key-value pair wouldn't be a string?
  • You do .Where and then .FirstOrDefault: replace this by .FirstOrDefault(y => y.Key == variable).

If you need to fetch a specific value from a DataRow, why not extract such code to a method?

private string GetValue(DataRow row, string key)
{
    return row[key].ToString();
}

Avoid doing Dictionary.ContainsKey: if you need to use the value of said key, use Dictionary<TKey, TValue>.TryGetValue.

var row = dt.Rows[i];
var languageId = GetValue(row, "LangId");

Dictionary<string,string>() contentByVariable;
if(!m_LUApplicationParams.TryGetValue(languageId, out contentByVariable))
{
    contentByVariable = new Dictionary<string,string>();
}

contentByVariable.Add(GetValue(row, "Variable"), GetValue(row, "Content"));
m_LUApplicationParams[languageId] = contentByVariable;

But if I were you I'd rethink this entire logic. A Dictionary<string, Dictionary<string, string>> is ugly and unwieldy; IMHO the nested dictionary should be a custom class.

I also feel this code is odd and possible buggy: data is retrieved for an applicationId yet this parameter is completely absent in m_LUApplicationParams. The GuidId that is passed is assumed to be present as a key in m_LUApplicationParams, same for the variable.

\$\endgroup\$
1
\$\begingroup\$

I would make this into a class that is dependency injected as singleton as static's are just a nightmare when it comes to threading and you want different lookups for different applicationId's (your current code doesn't do this). I will use unity in this example as the dependency injection library. (http://unity.codeplex.com/). Also your current method violates the single responsibility principle in my opinion as its doing to much. You don't want to do a where over the items of the dictionary, that is very expensive. Unlike the other answer I don't have a problem with the nested dictionary as it's encapsulated.

First I made an ApplicationParameterLookup class who's sole responsibility is to keep a dictionary for an application id. Then I added a static Lazy, this will keep the default container if the implementing code doesn't want to pass it's own into the static methods. I then added RegisterApplication which instanstiates ApplicationParameterLookup and adds it as an instance to a UnityContainer, this will make it as singleton and on Resolve the load logic will not be called again.

Then I added GetApplicationParameter which takes an application id, guid and variable and will attempt to resolve the ApplicationParameterLookup based on application id, if it can't find one it will register a new one. And finally it calls the instance's GetApplicationParameter.

public class ApplicationParameterLookup
{
    private Dictionary<string, Dictionary<string, string>> LookupParams = new Dictionary<string, Dictionary<string, string>>();
    public ApplicationParameterLookup(int applicationID)
    {
        LookupParams = new Dictionary<string, Dictionary<string, string>>();
        DataTable dt = null; //Get from repository

        foreach (DataRow row in dt.Rows)
        {
            string langId = row["LangId"].ToString();
            Dictionary<string, string> valueDictionary;
            if(!LookupParams.TryGetValue(langId, out valueDictionary))
            {
                valueDictionary = new Dictionary<string,string>();
                LookupParams[langId] = valueDictionary;
            }
            valueDictionary.Add(row["Variable"].ToString(), row["Content"].ToString());
        }
    }

    public string GetApplicationParameter(string guid, string variable)
    {
        Dictionary<string, string> valueDictionary;
        if (LookupParams.TryGetValue(guid, out valueDictionary))
        {
            string variableValue;
            if (valueDictionary.TryGetValue(variable, out variableValue))
                return variableValue;
        }
        return null;
    }
    private static Lazy<IUnityContainer> _containerInstance = new Lazy<IUnityContainer>(() => new UnityContainer());
    private static IUnityContainer ApplicationParameterContainer
    {
        get { return _containerInstance.Value; }
    }
    public static ApplicationParameterLookup RegisterApplication(int applicationID, IUnityContainer container = null)
    {
        if (container == null)
            container = ApplicationParameterContainer;
        ApplicationParameterLookup lookup = null;
        lock (container)
        {
            if (!container.IsRegistered<ApplicationParameterLookup>(applicationID.ToString()))
            {
                lookup = new ApplicationParameterLookup(applicationID);
                container.RegisterInstance<ApplicationParameterLookup>(applicationID.ToString(), lookup);
            }
            else
                lookup = container.Resolve<ApplicationParameterLookup>(applicationID.ToString());
        }
        return lookup;
    }        
    public static string GetApplicationParameter(int applicationID, string guid, string variable, IUnityContainer container = null)
    {
        if (container == null)
            container = ApplicationParameterContainer;
        var lookup = container.Resolve<ApplicationParameterLookup>(applicationID.ToString());
        if (lookup == null)
            lookup = RegisterApplication(applicationID, container);
        return lookup.GetApplicationParameter(guid, variable);
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.