Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
added 75 characters in body
Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

I have used a similiar approach at a time I needed it and in my opinion it is a valid way. That beeing said, let us take a look at the way you have implemented this.

public void SetAttr<T>(string name, T val)
{
 var t = typeof(T);
 if (attributes.ContainsKey(t))
 ((Dictionary<string, T>)attributes[t])[name] = val;
 else
 attributes[t] = new Dictionary<string, T>() { { name, val } };
}

The first improvement you should make is to avoid the call to the ContainsKey() method. Performance wise it is better to use TryGetValue() over ContainsKey().

See also: what-is-more-efficient-dictionary-trygetvalue-or-containskeyitem

The next I would always do is use brecaes {} for single commend if..else..else if statements to avoid stupid bugs.

Implementing the above would then lead to

public void SetAttr<T>(string name, T val)
{
 var t = typeof(T);
 
 object obj;
 if (attributes.TryGetValue(t, out obj))
 {
 Dictionary<string, T> dictionary = obj as Dictionary<string, T>;
 dictionary[name] = val;
 }
 else
 {
 attributes[t] = new Dictionary<string, T>() { { name, val } };
 }
}

which makes it more clear what the object in the attributes dictionary is.


What should happen if you try to retrieve a value form the container which isn't present ? How about adding something like a TryGetValue<T>() method to the container class.

Something like so

public bool TryGetAtrribute<T>(string name, out T val)
{
 object obj;
 if (attributes.TryGetValue(typeof(T), out obj))
 {
 Dictionary<string, T> dictionary = obj as Dictionary<string, T>;
 if (dictionary != null)
 {
 val = dictionary[name];
 return true;
 }
 }
 val = default(T);
 return false;
} 

In this way it is more obvious for a different developer that the GetAttr<T>() method can throw an exception.

Talking about exception, I would consider to throw an AttributeNotFoundException for the case that the desired value isn't in the container.

I have used a similiar approach at a time I needed it and in my opinion it is a valid way. That beeing said, let us take a look at the way you have implemented this.

public void SetAttr<T>(string name, T val)
{
 var t = typeof(T);
 if (attributes.ContainsKey(t))
 ((Dictionary<string, T>)attributes[t])[name] = val;
 else
 attributes[t] = new Dictionary<string, T>() { { name, val } };
}

The first improvement you should make is to avoid the call to the ContainsKey() method. Performance wise it is better to use TryGetValue() over ContainsKey().

See also: what-is-more-efficient-dictionary-trygetvalue-or-containskeyitem

The next I would always do is use brecaes {} for single commend if..else..else if statements to avoid stupid bugs.

Implementing the above would then lead to

public void SetAttr<T>(string name, T val)
{
 var t = typeof(T);
 
 object obj;
 if (attributes.TryGetValue(t, out obj))
 {
 Dictionary<string, T> dictionary = obj as Dictionary<string, T>;
 dictionary[name] = val;
 }
 else
 {
 attributes[t] = new Dictionary<string, T>() { { name, val } };
 }
}

which makes it more clear what the object in the attributes dictionary is.


What should happen if you try to retrieve a value form the container which isn't present ? How about adding something like a TryGetValue<T>() method to the container class.

Something like so

public bool TryGetAtrribute<T>(string name, out T val)
{
 object obj;
 if (attributes.TryGetValue(typeof(T), out obj))
 {
 Dictionary<string, T> dictionary = obj as Dictionary<string, T>;
 val = dictionary[name];
 return true;
 }
 val = default(T);
 return false;
} 

In this way it is more obvious for a different developer that the GetAttr<T>() method can throw an exception.

Talking about exception, I would consider to throw an AttributeNotFoundException for the case that the desired value isn't in the container.

I have used a similiar approach at a time I needed it and in my opinion it is a valid way. That beeing said, let us take a look at the way you have implemented this.

public void SetAttr<T>(string name, T val)
{
 var t = typeof(T);
 if (attributes.ContainsKey(t))
 ((Dictionary<string, T>)attributes[t])[name] = val;
 else
 attributes[t] = new Dictionary<string, T>() { { name, val } };
}

The first improvement you should make is to avoid the call to the ContainsKey() method. Performance wise it is better to use TryGetValue() over ContainsKey().

See also: what-is-more-efficient-dictionary-trygetvalue-or-containskeyitem

The next I would always do is use brecaes {} for single commend if..else..else if statements to avoid stupid bugs.

Implementing the above would then lead to

public void SetAttr<T>(string name, T val)
{
 var t = typeof(T);
 
 object obj;
 if (attributes.TryGetValue(t, out obj))
 {
 Dictionary<string, T> dictionary = obj as Dictionary<string, T>;
 dictionary[name] = val;
 }
 else
 {
 attributes[t] = new Dictionary<string, T>() { { name, val } };
 }
}

which makes it more clear what the object in the attributes dictionary is.


What should happen if you try to retrieve a value form the container which isn't present ? How about adding something like a TryGetValue<T>() method to the container class.

Something like so

public bool TryGetAtrribute<T>(string name, out T val)
{
 object obj;
 if (attributes.TryGetValue(typeof(T), out obj))
 {
 Dictionary<string, T> dictionary = obj as Dictionary<string, T>;
 if (dictionary != null)
 {
 val = dictionary[name];
 return true;
 }
 }
 val = default(T);
 return false;
} 

In this way it is more obvious for a different developer that the GetAttr<T>() method can throw an exception.

Talking about exception, I would consider to throw an AttributeNotFoundException for the case that the desired value isn't in the container.

Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

I have used a similiar approach at a time I needed it and in my opinion it is a valid way. That beeing said, let us take a look at the way you have implemented this.

public void SetAttr<T>(string name, T val)
{
 var t = typeof(T);
 if (attributes.ContainsKey(t))
 ((Dictionary<string, T>)attributes[t])[name] = val;
 else
 attributes[t] = new Dictionary<string, T>() { { name, val } };
}

The first improvement you should make is to avoid the call to the ContainsKey() method. Performance wise it is better to use TryGetValue() over ContainsKey().

See also: what-is-more-efficient-dictionary-trygetvalue-or-containskeyitem

The next I would always do is use brecaes {} for single commend if..else..else if statements to avoid stupid bugs.

Implementing the above would then lead to

public void SetAttr<T>(string name, T val)
{
 var t = typeof(T);
 
 object obj;
 if (attributes.TryGetValue(t, out obj))
 {
 Dictionary<string, T> dictionary = obj as Dictionary<string, T>;
 dictionary[name] = val;
 }
 else
 {
 attributes[t] = new Dictionary<string, T>() { { name, val } };
 }
}

which makes it more clear what the object in the attributes dictionary is.


What should happen if you try to retrieve a value form the container which isn't present ? How about adding something like a TryGetValue<T>() method to the container class.

Something like so

public bool TryGetAtrribute<T>(string name, out T val)
{
 object obj;
 if (attributes.TryGetValue(typeof(T), out obj))
 {
 Dictionary<string, T> dictionary = obj as Dictionary<string, T>;
 val = dictionary[name];
 return true;
 }
 val = default(T);
 return false;
} 

In this way it is more obvious for a different developer that the GetAttr<T>() method can throw an exception.

Talking about exception, I would consider to throw an AttributeNotFoundException for the case that the desired value isn't in the container.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /