How can I clean up this code?

Question

I want to make my code look cleaner and 'express its intent' more..

The following method is reflecting over two types and checking that if there is a property in one type, then there is a corresponding property of the same name in the other type. If there is then it sets the value of the property to the index variable. otherwise it throws an exception which is caught and logged from the calling code.

Watching videos and looking through other code bases its sometimes easy to see at a glance what the code is doing but im not happy with this method as I feel at first glance it just looks like a mess and to understand it you really have to read it through rather than glance at it. Does anyone have any tips to improve readability etc?

    private void GenerateFieldIndexes()
    {
        Type thisType = GetType();
        Type tnsType = typeof (T);

        Dictionary<string, PropertyInfo> indexInfos = new Dictionary<string, PropertyInfo>();

        string formattedName;

        // We need to start from 2 as first two fields are always hard coded
        int index = 2;

        foreach (PropertyInfo property in tnsType.GetProperties())
        {
            formattedName = property.Name.Trim().ToLower();
            indexInfos[formattedName] = property;
        }

        PropertyInfo p;
        foreach (PropertyInfo property in thisType.GetProperties())
        {
            formattedName = property.Name.Trim().ToLower();

            if (!indexInfos.ContainsKey(formattedName))
                throw new FieldNameMissingException(string.Format("{0} Property name {1} missing!", GetType(),
                                                                  formattedName));
            p = GetProperty(thisType, formattedName);

            if (p == null)
                throw new FieldNameMissingException(string.Format("{0} Property name {1} missing!", GetType(),
                                                                  formattedName));
            p.SetValue(thisType, index, null);
            FieldNames.Add(formattedName);
            index++;
        }
    }
    private PropertyInfo GetProperty(Type t, string propertyName)
    {
        return t.GetProperty(propertyName, BindingFlags.Instance | BindingFlags.Public | BindingFlags.IgnoreCase);
    }

Answer 1

You need to start by naming the method to reflect what it actually does; GenerateFieldIndexes could mean many things to many people.

Passing in parameters, such as the source type and target type might also be helpful.

AssignMatchingProperties(source, target);

Answer 2

Here's a start:

private void GenerateFieldIndexes()
{
    Type thisType = GetType();
    Type tnsType = typeof (T);

    Dictionary<string, PropertyInfo> indexInfos = IndexPropertyInfos(tnsType);

    foreach (PropertyInfo property in thisType.GetProperties())
    {
        string formattedName = FormatName(property);

        if (!indexInfos.ContainsKey(formattedName))
            throw new FieldNameMissingException(string.Format("{0} Property name {1} missing!", GetType(),
                                                              formattedName));
        PropertyInfo p = GetProperty(thisType, formattedName);

        if (p == null)
            throw new FieldNameMissingException(string.Format("{0} Property name {1} missing!", GetType(),
                                                              formattedName));
        p.SetValue(thisType, index, null);
        FieldNames.Add(formattedName);
        index++;
    }
}

private PropertyInfo GetProperty(Type t, string propertyName)
{
    return t.GetProperty(propertyName, BindingFlags.Instance | BindingFlags.Public | BindingFlags.IgnoreCase);
}

private Dictionary<string, PropertyInfo> IndexPropertyInfos(Type type)
{
    var indexedPropertyInfos = new Dictionary<string, PropertyInfo>();

    // We need to start from 2 as first two fields are always hard coded
    int index = 2;

    foreach (PropertyInfo property in type.GetProperties())
    {
        string formattedName = FormatPropertyName(property);
        indexInfos[formattedName] = property;
    }

    return indexedPropertyInfos;
}

private string FormatName(PropertyInfo property)
{
    return property.Name.Trim().ToLower();
}

I recommend that you do not introduce variables outside a loop if you don't need to use return value outside as well. Introducing it outside does not result in performance increases and new objects are created and assigned to it inside the loop.

Also you introducte a thisType variable at the start of GenerateFieldIndexes (which by the way should be GenerateFieldIndices) with the value of GetType() but you still use GetType() directly later on in the method.

I think that one of the most important lessons in cleaning code that I've learned is that only write methods that are small (they fit on a screen) and they do just one thing which is properly described in the name of the method.

If you want to find out more about writing clean code, I recommend that you read Uncle Bob's (Robert C. Martin) Clean Code: probably the most important programming book I've ever read.

Answer 3

1. You should use interface if you want to make sure a class implements a certain set of functions. Using interface, the compiler will check whether your class matches another class at compile time. Using interface, the ugly code you're writing then become trivial.
2. You should change your requirement; don't allow case insensitive matching.
3. If your code really need this sort of thing and you don't have a way out, then don't use a static language. Use a dynamic language that allows monkey patching, duck typing, and other sorts of techniques. Languages in this category includes Python and Ruby.

Answer 4

To me, what makes this method undecipherable is the fact that it's using a number of global (or class) variables with no reference to where they come from or what they are for. Apparently you have a global variable named "T", which could mean just about anything. There is no amount of syntactical context that would give T any meaning.

The first thing you should do is change all your global (or class) variable names so that they will have meaning in any context where they are used.

I also find that a few comments can do wonders to improve readability. I always like to put a brief comment on top of any non-trivial loop. You might add the following just before your second loop:

/* The following loop checks that every property in thisType has a corresponding property of
 * the same name in tnsType. If any mismatches are found, an exception is thrown. For each
 * matching property, the value of the property is set to the index variable. 
 */

Of course, a comment on top of the method itself is useful, especially if you use javadoc. Then for some IDE users (like eclipse), they can just hover over calls to your method and see a description of the parameters, return values, and what the method does, like so:

/**
 * This method checks to make sure that all the properties match.
 * <p>For each match found, the value of the property is set to the index variable.</p>
 * @throws FieldNamemissingException if any mismatches are found.
 */

I would also recommend renaming your local and scope variables if possible to be a bit more descriptive of the actions and intent. For example, p, thisProperty and indexInfos don't tell me much. There's no penalty for longer variable names.

Answer 5

The following method is reflecting over two types and checking that if there is a property in one type, then there is a corresponding property of the same name in the other type. If there is then it sets the value of the property to the index variable. otherwise it throws an exception which is caught and logged from the calling code.

Some problems I see here:

• The reason the code is hard to follow is in large part because the specification of the method indicates that it does a whole lot of things. Do less per method.

• The method is insufficiently general. It does not reflect over any two types; it reflects over two specific types, the type of "this" and the type T. It is therefore impossible to factor this thing away from its current implementation.

• The action of the method is to induce multiple side effects. Side effecting methods are hard to reason about. You have to make sure you call them exactly the right number of times (usually once). They're thereby hard to unit test.

• the correctness of the side effects - multiple mutations - depends on the author of this function having special knowledge about the thing being mutated. The fact that there is a comment in there that says "make sure you start at two" is indicative of how this method is tied explicitly to some data structure outside of it.

• This method is highly suspect because it can both induce mutations and throw exceptions. You can get 90% of the way there, happily setting properties on objects and filling in tables when suddenly, boom, you've thrown an exception. Is that correct? Is that desirable? It seems potentially awful; you've now got all these objects in a partially mutated state and an unexpected exception moving up the stack. (In general this is unavoidable; exceptions can happen almost anywhere. But don't make it worse. If you can know ahead of time that you are going to throw an exception then do so before you induce a mutation. Don't do a lot of work and then fail; fail early.)

• Incidentally, the exception logic seems wrong. You put the same type in both exceptions, regardless of which type is missing the property.

• The method is not correct if there are multiple properties that differ only in case. I assume that you are explicitly not in this situation; if you are, then you need to write different logic that handles that case correctly.

• What's all this messing around with dictionaries for? A Type object is already a dictionary that maps strings onto properties; why are you building a copy of the information that is already in a Type object that you've got? Just ask the Type for the property whenever you need it.

• the reflection call that sets the value seems wrong; why are you passing the type as the receiver???

The fundamental technique I would use to fix this mess is to try to isolate the effect-causing stuff to a small number of places, and then make helper functions that are actually functions - that give a result based on their inputs, without side effects.

I haven't compiled this; this is just off the top of my head. But I would probably write your function something like this:

// This method first verifies that the source and destination have compatible types.
// If they do not then an exception is thrown.
// Otherwise, the name of each property is appended to the list, and each
// property of the destination is set to the index of its name on the list.

private static void GenerateFieldIndexes(Type sourceType, object destination, IList<string> names)
{
    Debug.Assert(sourceType != null);
    Debug.Assert(destination != null);
    Debug.Assert(names != null);

    // This throws an exception if the types are bad:
    var destType = destination.GetType();
    VerifyCommonPropertyNames(sourceType, destType);

    foreach (var sourceProp in sourceType.GetProperties()) 
    { 
        var formattedName = sourceProp.Name.FormatName();
        var destProp = GetProperty(destType, formattedName);
        Debug.Assert(destProp != null);
        destProp.SetValue(destination, names.Count, null); 
        names.Add(formattedName); 
    } 
} 

// Throw an exception if the two types do not have the right properties.
private static void VerifyCommonPropertyNames(Type sourceType, Type destType) 
{ 
    Debug.Assert(sourceType != null);
    Debug.Assert(destType != null);

    // We must have the same property names in both types, modulo case.

   // (If you want to throw a different exception based on whether the missing
   // property is in sourceType or destType, then use different set intersection
   // logic to figure out which case you're in.)

    HashSet<string> names = sourceType.GetFormattedPropertyNames();
    HashSet<string> badNames = destType.GetFormattedPropertyNames();
    badNames.SymmetricExceptWith(names);

   // badNames now contains all the names that were in sourceType but not destType,
   // or in destType but not in sourceType.

   if (badNames.Any())
       throw something based on badNames.First();
}

private static HashSet<string> GetFormattedPropertyNames(this Type type)
{
    Debug.Assert(type != null);
    return new HashSet<string>(
        from property in type.GetProperties()
        select property.Name.FormatName());
}

private static PropertyInfo GetProperty(Type t, string propertyName) 
{ 
    Debug.Assert(type != null);
    Debug.Assert(name != null);
    return type.GetProperty(name, BindingFlags.Instance | BindingFlags.Public | BindingFlags.IgnoreCase); 
} 

private static string FormatName(this string name)
{
    Debug.Assert(name != null);
    return name.Trim().ToLower();
} 

Answer 6

Identifying exactly what your specifications are would help. From what you've said, and from what the code appears to be doing, you just want to take each property on a given type and verify that it's on another type, then set your own type's property with an index value if it is. From that perspective (and perhaps there are additional requirements that I'm unaware of), your code is jumping through a few unnecessary hoops and could be condensed down to something more manageable.

• The first loop cycles through each property in tnsType and loads it into a local Dictionary.
• The second loop checks the existence of a Dictionary value, ignores the PropertyInfo that you're currently working with and reloads it from the type.

This could be consolidated down to a single loop with a get and a set (unless you have other requirements that may impact this). It's also important to note that GetProperties() is not guaranteed to return properties in any particular order, so if your code depends on the order (and I suspect that's true because the code seems to be equating a given index with a given property) you may end up getting unexpected results at some point.

private void GenerateFieldIndexes()
{
    Type thisType = GetType();
    Type tnsType = typeof(T);

    int index = 2;

    foreach (PropertyInfo property in thisType.GetProperties())
    {                    
        var tnsProperty = GetProperty (tnsType, property.Name);

        if (tnsProperty == null)
                    throw new FieldNameMissingException(string.Format("{0} Property name {1} missing!", thisType, property.Name));                   

        property.SetValue(this, index, null);
        FieldNames.Add(property.Name.ToLower());
        index++;
    }
}

Answer 7

For readability, always ask yourself whenever you do a "foreach" if using Linq would make more sense... e.g. for the first part:

Dictionary<string, PropertyInfo> indexInfos = typeof(T)
        .GetProperties()
        .ToDictionary(property => property.Name.Trim().ToLower());