7

What are the common bugs introduced by refactoring, carelessness, and newbies?

I would like to request the experienced programmers here to share their experience and list the bugs they used to introduce when they were inexperienced.

In your response, please write a headline mentioning the kind of bug in bold text, followed by few linebreaks, and then an explanation, cause of the bug, and finally the fix.

2
  • 2
    It's interesting that both bugs so far could have been caught by the type checker if the language/framework had been designed differently. Dec 2, 2010 at 17:58
  • Yes, that's why I said without linq. The new linq based O/RM don't have this issue, so things are getting better.
    – BlackICE
    Dec 2, 2010 at 19:27

3 Answers 3

3

Manually written values instead of constants

Example:

public District GetDefaultDistrict() {
  return GetById(1);
}

public IList<Revenue> GetRevenues() {
  return GetByCodes(new [] { 8010, 8011, 8096 });
}

and thousands of use of 1, 8010, 8011 and 8096 in other places. Try to image if the default district now is 2 and 8011 moved to 8012.

Fix:

public District GetDefaultDistrict() {
  return GetById(Consts.DEFAULT_DISTRICT_ID);
}

public IList<Revenue> GetRevenues() {
  return GetByCodes(Consts.REVENUE_CODES);
}

and use this constants everywhere where you need to determine default district id and/or other static values.

Or even:

public IList<Revenue> GetRevenues() {
  var codes = GetRevenueCodes(); // get from db
  return GetByCodes(codes);
}

to get actual values from db. But this is just an example.

1
  • 2
    'Magic numbers' would have sufficed :D
    – Nobody
    Dec 2, 2010 at 21:45
2

Property/String mismatch namely when using O/RM frameworks without linq:

    Dim crit As DetachedCriteria
    crit = DetachedCriteria.For(GetType(RemitLineItemEntity), "remit")

    If Not String.IsNullOrEmpty(LastName) Then
        If LastName.Contains("%") Then
            crit = crit.Add(New LikeExpression("LastName", LastName))
            countCrit = countCrit.Add(New LikeExpression("LastName", LastName))
        Else
            crit = crit.Add(Expression.Eq("LastName", LastName))
            countCrit = countCrit.Add(Expression.Eq("LastName", LastName))
        End If

    End If

Then changing the LastName property on the object to SurName, or such.

Fix

Only one I know of is to use a tool to do refactoring (which most do). They should catch the quoted field name as well, but usually use human verification on it (which leads to human error).

1
  • There are non-linq O/RM query engines (Entity Spaces comes to mind) that didn't require you to use literal strings for the column names. It provided constants for all the columns/properties. Dec 2, 2010 at 19:41
1

INotifyPropertyChanged bug introduced by refactoring, misspelling, case-mismatch etc.

Sometimes we change the name of some of our properties, and we forget to change the "string" we pass toOnPropertyChanged() method (especially when we call this method from somewhere else), or simply misspell it, including case-mismatch.

Something like this:

public string FirstName //earlier it was simply 'Name'
{
      get { return m_name; }
      set 
      { 
          m_name = value ;
          OnPropertyChanged("Name"); //still 'Name'. Bug!
         //or, OnPropertyChanged("FirstNane"); //changed, but misspelled. Bug!
      }
}

Fix

protected void OnPropertyChanged(string propertyName)
{
   //this raises exception if there is something wrong (only in debug mode!).
   RuntimeAssert.ValidatePropertyName(this, propertyName); 

   //your code here
}

Here is the implementation of RuntimeAssert class.

   public static class RuntimeAssert
    {
        private static Dictionary<Type, List<string>> ClassPropertyMap = new Dictionary<Type, List<string>>();
        private static List<string> GetProperties(Type type)
        {
            if (!ClassPropertyMap.ContainsKey(type))
            {
                PropertyInfo[] props = type.GetProperties(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static);
                ClassPropertyMap.Add(type, new List<string>(props.Where(p => true).Select(p => p.Name)));
            }
            return ClassPropertyMap[type];
        }
        [Conditional("DEBUG")]
        public static void ValidatePropertyName(object instance, string propertyName)
        {
            ValidatePropertyName(instance.GetType(), propertyName);
        }
        [Conditional("DEBUG")]
        public static void ValidatePropertyName(Type type, String propertyName)
        {
            List<string> properties = RuntimeAssert.GetProperties(type);
            if (!properties.Contains(propertyName))
            {
                string message = String.Format("Property '{0}' not found in class '{1}'", propertyName, type.FullName);
                throw new PropertyNotFoundException(message);
            }
        }
    }

And finally, PropertyNotFoundException (it's used in above class).

public class PropertyNotFoundException : Exception
    {
        public PropertyNotFoundException(string message) : base(message) 
        {

        }
    }
6
  • I afraid your fix is very complex. I would prefer to make the class autogenerated. So there FirstName will be FirstName. Everywhere. This how I do most of my projects and I don't have similar problems during refactoring, before and after.
    – Genius
    Dec 2, 2010 at 18:54
  • I find it's more common to use some method of reflection to generate and cache the PropertyChangedEventArgs and string property names in a static variable at application startup and use that. Here's a helper class that'll let you do that. Btw, not sure why that file header still says LGPL, the whole Utilities library was changed to an MIT license. I will check on that. Dec 2, 2010 at 19:38
  • @Genius.... could you please elaborate on what you said? I couldn't really understand that. Or maybe, you would like to add one entry here, explaining how you would pefer. :-)
    – Nawaz
    Dec 3, 2010 at 5:33
  • @Nawaz, sure. I 'll not add an entry here since it doesn't fit to the subject, mb in other topic. So what did I mean: as I see your property, this is a part of entity. Entity is probably from db. Usually we create classes that maps to tables w/fields. What I was saying about is: Not to create classes w/props manually, but create a template for them in CodeSmith (comm) or T4 (free) and use every time when the database is changed, so it will create classes (entities) and properties (from fields) automatically. Hope my explanation is clear.
    – Genius
    Dec 3, 2010 at 6:05
  • @Genius... thanks for the explanation.. But your scenario is very specific, namely when you create (autogenerate) your classes that map to some entities in database. So here autogeneration makes perfect sense, in fact we do it all the time, like LINQ to SQL.. but what if you write your own class, like presenter/model (in MVP pattern), you don't really have autogeneration option then. You've to create your class and all it's properties manually. :-)
    – Nawaz
    Dec 3, 2010 at 6:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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