Keep database lookup in memory and get values in frequent

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

static Dictionary> 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>();`

                    for (int i = 0; i ());
                        }

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

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

• 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 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.TryGetValue.

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

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

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

StackExchange Code Review Q#82266, answer score: 2