current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I was wondering if this was good design from client side calls. In the event that the key doesn't exist, go ahead and add it. 

public static void SetVariable(string name, object value)
{
    if (variables == null)
    {
        variables = new Dictionary<string, object>();
    }
    if(variables.ContainsKey(name))
    {
        variables[name] = value;    
    }
    else
    {
        Debug.LogError("Added Unknown Data to Global Data");
        variables.Add(name,value);
    }


    //EmberDebug.Log( name + " = "+ value );
}
share|improve this question

closed as off-topic by Jamal♦ Feb 21 '14 at 18:01

If this question can be reworded to fit the rules in the help center, please edit the question.
    
I would use a ConcurrentDictionary for this bevouse it has two AddOrUpdate which are very useful and this class also threadsafe. –  Peter Kiss May 4 '13 at 5:59
    
Thank you! That is great advice! –  10001110101 May 4 '13 at 6:59
    
Or simply use the indexer of Dictionary, that would work too. –  svick May 4 '13 at 12:32
    
Agree with svick. I don't think doing a Lazy Initialization in a Set function is good, and that variables should be initialized before (preferably in the constructor). What do you think will happen if you try to Get and your dictionary is null? –  Max Dec 20 '13 at 7:58
    
This question appears to be off-topic because it is seeking a design review and not a code review. –  Jamal♦ Feb 21 '14 at 18:01

1 Answer 1

up vote 3 down vote 
if (variables == null)
{
    variables = new Dictionary<string, object>();
}

I think you shouldn't repeat this code whenever you want to use variables. So, just make sure variables is never null by initializing it in the constructor.

if(variables.ContainsKey(name))
{
    variables[name] = value;    
}
else
{
    Debug.LogError("Added Unknown Data to Global Data");
    variables.Add(name,value);
}

If the key doesn't exist, then the indexer works the same as Add(), so I would simplify your code to:

if (!variables.ContainsKey(name))
{
    Debug.LogError("Added Unknown Data to Global Data");
}

variables[name] = value;

If you later decide to remove the logging, it would simplify your code even more. Also, I would suggest to also log the name of the added variable, I think that would make the information in your log more useful.

share|improve this answer
    
Do you mean that if "name" isn't present and you use the indexer with the unknown value, the Dictionary will add it automatically ? –  user24499 May 4 '13 at 13:17
1  
@JoanLeaven Yeah, that's what it's documented to do: “You can also use the [indexer] to add new elements by setting the value of a key that does not exist in the Dictionary<TKey, TValue>.” –  svick May 4 '13 at 14:13
    
Heh ! Can you imagine ... I didn't know that. +1. –  user24499 May 4 '13 at 15:17

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