views:

175

answers:

3

I've got the following code where for some reason I'm getting a KeyNotFoundException even though I'm using a key that I had retrived a couple of lines above. Does anyone know of a situation where this wouldn't work? I'm stumped. BTW 'SchemaElementType is an enum.

public class DefaultValue
{
 private Dictionary<Parameter, string> _params;

 public DefaultValue(Dictionary<Parameter, string> parameters)
 {
        _params = parameters;
 }

  public string GetParameterValue(string name)
  {
      foreach(Parameter param in _params.Keys)
      {
           if(param.ParamName.Equals(name))
           {
               // **** Issue here  ****
               return _params[param];
           }
      }
      return string.Empty;
  }
}

[DataContract]
public class Parameter
    {
        #region Members
        private Guid _guid;
        private Guid _formulaGuid;
        private string _name;

        #endregion

        #region Constructor
        public Parameter(Guid guid, Guid formulaGuid, string name, SchemaElementType type)
        {
            ParamGuid = guid;
            FormulaGuid = formulaGuid;
            ParamName = name;
            ParamType = type;
        }

        public Parameter()
        {}

        #endregion

        #region Properties

        [DataMember]
        public Guid ParamGuid
        {
            get { return _guid; }
            set { _guid = value; }
        }

        [DataMember]
        public Guid FormulaGuid
        {
            get { return _formulaGuid; }
            set { _formulaGuid = value; }
        }

        [DataMember]
        public string ParamName
        {
            get { return _name; }
            set { _name = value; }
        }

        [DataMember]
        public SchemaElementType ParamType { get; set; }

        #endregion

        #region Overrides

        public bool Equals(Parameter other)
        {
            if (ReferenceEquals(null, other)) return false;
            if (ReferenceEquals(this, other)) return true;
            bool result =other._guid.Equals(_guid);
            result = result && other._formulaGuid.Equals(_formulaGuid);
            result = result && Equals(other._name, _name);
            result = result && Equals(other.ParamType, ParamType);

            return result;
        }

        public override int GetHashCode()
        {
            unchecked
            {
                int result = _guid.GetHashCode();
                result = (result*397) ^ _formulaGuid.GetHashCode();
                result = (result*397) ^ (_name != null ? _name.GetHashCode() : 0);
                result = (result*397) ^ ParamType.GetHashCode();
                return result;
            }
        }

        public override bool Equals(object obj)
        {
            if (ReferenceEquals(null, obj)) return false;
            if (ReferenceEquals(this, obj)) return true;
            if (obj.GetType() != typeof (Parameter)) return false;
            return Equals((Parameter) obj);
        }

        #endregion
    }
+1  A: 

Well strictly speaking you're not retrieving the (lookup key) a few lines above you are retreiving an object that at some point was used for calculating the hash key.

When you insert into a Dictionary the GetHashKey method of the key object will be called. If that changes from the time you insert the key-value pair to the time your code is executed you will get the described behaviour. (unless of cause if the GetHashKey no returns a value matching a key to a different key-value pair, in that case you get really weird behaviour not an exception)

I'd look for the value of the hash key when inserting and when retreiving and see if there's a mismatch between them

Rune FS
<quote>changes from the type you..</quote> should that not be time? i believe that is what you meant?
Wayne
@Wayne you corrected and i've made the change tu
Rune FS
A: 

You can take advantage of the KeyValuePair<> class:

foreach(var item in _params)
{
   if(item.Key.ParamName.Equals(name))
   {
      return item.Value;
   }
}
Douglas Tosi
hmm, var abuse :) I would stick to using the KeyValuePair<Parameter,string> as var should only be used when you don't know the type (as per the C# reference), but I guess thats another debate that happens elsewhere on SO :)
Wayne
@Wayne - this is obviously a subjective point, but what extra information would that tell you? Why clutter up the code? IMO the `var` usage is perfectly clear and acceptable here.
Marc Gravell
I've marked this as the answer as it helped me get the bug fixed (tight schedule) though I have given votes to both Mark and Rune FS as they both highlighted the need to improve the coding of the Parameter class/ use of it as key. I will be getting round to introducing these changes asap. Thanks guys.
Martin MacPherson
+2  A: 

I worry about the fact that Parameter is mutable. If (after adding it to the dictionary) you have changed any of the values that are used when generating GetHashCode() (i.e. all of them) then all bets are off and you are not guaranteed to see your item again. I would not make these public setters, i.e.

    [DataMember]
    public string ParamName // applies to all the properties, not just this one
    {
        get { return _name; }
        private set { _name = value; }
    }

Actaully, I'd probably drop the explicit fields and use C# 3.0 automatically implemented properties:

    [DataMember]
    public string ParamName { get; private set; }

As an example that breaks by changing the parameter:

    var data = new Dictionary<Parameter, string>();
    Parameter p;
    data.Add((p = new Parameter(Guid.NewGuid(), Guid.NewGuid(), "abc",
        SchemaElementType.A)), "def");
    var dv = new DefaultValue(data);
    string val1 = dv.GetParameterValue("abc"); // returns "def"
    p.ParamGuid = Guid.NewGuid();
    string val2 = dv.GetParameterValue("abc"); // BOOM

As a final thought; if the typical usage is to lookup by string, then why not use the name as the key for the internal dictionary? At the moment you aren't using the dictionary properly.

Marc Gravell
I'm pretty sure that the Parameter key isn't changed at all between the add and get but I will check. I've only just picked up the code from someone else so not totally aware of its use yet. Totally agree with your thoughts on the implementation, just need to access the impact of making changes.
Martin MacPherson