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.

So i have a super class that has a 3 child classes. Those 3 child classes have multiple classes of their own. I was having troubles saving this into XML, so I decided to parse it out and do a little bit of maths to return the correct subclass type and make an instance of it. So after a little bit of digging it appears that the "easiest" method was to use reflection and LINQ . I'm new to LINQ (I've been very stubborn to use it). It appears to me to be something similar to SQL, which makes me think I'm doing this wrong (even though it works according to my unit tests)

internal static CustomTask GetTask(string type)
{
    var types = Assembly.GetAssembly(typeof(CustomTask)).GetTypes().Where(t => t.IsSubclassOf(typeof(CustomTask)));
    Globals.WriteLog(types.ToString());
    foreach (Type t in types)
    {
        Globals.WriteLog(t.FullName);
        if (t.FullName.Contains(type))
        {
            Globals.WriteLog("^^^^^^^^^^FOUND IT^^^^^^^^^^");
            var tasktype = Type.GetType(t.FullName);
            return (CustomTask)Activator.CreateInstance(tasktype);
        }
    }
    Globals.WriteLog("Fell through to switch statement, not fully implemented");
    switch (type)
    {
        case "ScannerTask":
            return new ScannerTask();
        default:
            return new UnassignTask();
    }
}

I guess because I have unit tests I shouldn't be afraid to edit the LINQ statment, but it works, and it does seem rather clean. And I don't know what it would return if it didn't find a proper subclass (although it should seeing as the xml save method requires a CustomTask as part of its parameters) The switch statment is there to show what I was doing before I tried LINQ. So how can I make my LINQ statement only return the correct type so I can create a instance of it?

EDIT

Well this is what i'm down too. It passes all my tests, but still doesn't feel correct. thoughts?

    var types = Assembly.GetAssembly(typeof(CustomTask)).GetTypes();
    var subclasses = types.Where(t => t.IsSubclassOf(typeof(CustomTask)));
    var single = subclasses.Where(t => t.FullName.Contains(type)).Single();
    Globals.WriteLog(single.ToString());
    var tasktype1 = Type.GetType(single.ToString());
    return (CustomTask)Activator.CreateInstance(tasktype1);

EDIT2

ok so with the one suggestion I have this now. I like it.. but i have a sneaky suspicion it could be better yet..

internal static CustomTask GetTask(string type)
{
        var types = Assembly.GetAssembly(typeof(CustomTask)).GetTypes();
        var subclasses = types.Where(t => t.IsSubclassOf(typeof(CustomTask)));
        var single = subclasses.Single(t => t.FullName.Equals(type));
        Globals.WriteLog(single.ToString());
        return (CustomTask)Activator.CreateInstance(single);
}

public static void SaveEventListener(XmlDocument xmlDocument, AbstractEventListener akel, string filename)
{
    Globals.WriteLog("xml.SaveEventListener()+");
    string query = string.Format("//Key[@Name='{0}']", akel.key);
    var node = xmlDocument.SelectSingleNode(query);
    if (node == null) // no value found save new
    {
        Globals.WriteLog("Creating new key");
        XmlElement ele = xmlDocument.CreateElement("Key");
        ele.SetAttribute("Name", akel.key.ToString());

        var child = xmlDocument.CreateElement("Task");
        child.SetAttribute("Type", akel.Task.GetType().FullName);
        child.InnerText = akel.Task.GetOptionString();
        ele.PrependChild(child);

        xmlDocument.DocumentElement.PrependChild(ele);
    }
    else
    {
        //Need to implement a update yet.
    }
    xmlDocument.Save(filename);
    Globals.WriteLog("xml.SaveEventListener()-");
}

public static AbstractEventListener LoadEventListener(XmlDocument xmlDocument, System.Windows.Forms.Keys keys)
{
    Globals.WriteLog("xml.LoadEventListener()+");
    AbstractEventListener ret = new AbstractEventListener(keys, new UnassignTask());

    string query = string.Format("//Key[@Name='{0}']", keys);
    var node = xmlDocument.SelectSingleNode(query);
    if (node != null)
    {
        Globals.WriteLog("Found Key, parsing");
        var type = node.ChildNodes[0].Attributes["Type"].Value;
        var task = KeymonTask.GetTask(type);
        ret.Task = task;
        ret.Task.Parse(node.ChildNodes[0].InnerText);
    }
    Globals.WriteLog("xml.LoadEventListener()-");

    return ret;
}

forinstance in the load function I have ret = new AbstractEventListener(keys, new UnassignTask()); that portion could stand to be writen differently... well the whole method could stand a small fix I think.

share|improve this question
add comment

3 Answers

up vote 1 down vote accepted

In a single statement it can be rewritten as following (without logging though):

return Assembly.GetAssembly(typeof(CustomTask)).GetTypes()
    .Where(t => typeof(CustomTask).IsAssignableFrom(t) && t.FullName.Contains(type))
    .Select(t => (CustomTask)Activator.CreateInstance(t))
    .SingleOrDefault();

Note that Single method throws exception if lookup won't find the class matching your criteria. If you prefer to return null in this case - use SingleOrDefault instead.

Also, if this kind of code will be called quite often I would suggest to create a Dictionary that caches the mapping from previously looked up keywords (strings) to the type found.

But in general your solution doesn't look correct from architectural point of view: you're trying to match a certain string to full name of the class (and that string most likely is not equal to full name, otherwise you could just call Type.GetType() method). It may cause side effects if someone will add a class containing the same substring: for example your intention is to use "ScannerTask" string to lookup YourNamespace.ScannerTask class, and it works correctly now. But after a year of intense development someone may decide to introduce some new fancy task called MyNewNamespace.MuchBetterScannerTask... and bang, your code is broken, since new class may suddenly pop out instead of expected one.

Update Since you're saving a full name of the type you don't need to search all assembly types for it, just do

internal static CustomTask GetTask(string fullname)
{
    var type = Type.GetType(fullname);
    return type != null
        ? (CustomTask)Activator.CreateInstance(type)
        : new UnassignTask();
}
share|improve this answer
    
here in a second i'll post my save method, and maybe that will shed some more light into why i'm not entirely worried about the newer potential tasks. But you do bring up a valid point in that the way I am looking for it needs to be revised. How do I set the default to something else? because I do have a default of UnassignedTask which essentially is a null Task. –  Robert Snyder Feb 25 '13 at 19:24
    
That is a awesome update. I am actually going to use that answer instead. It is straight forward and easy. Thank you very much for your time. –  Robert Snyder Feb 25 '13 at 20:40
add comment

There are a few things I notice:

  • tasktype1 is redundant, since single is already of type System.Type, so there is no need to do the Type.GetType(single.ToString()); call. You can use single directly.
  • The .Where(t => t.FullName.Contains(type)).Single () call can instead be replaced with .Single (t => t.FullName.Contains(type))
  • The manner in which you match types could return incorrect results. t.FullName.Contains(type) will return any type where type is a substring of the fully-qualified type name. This can even include cases where type is a substring of one of the namespaces of your intended type. You may consider instead using t.Name == type.

That reduces the updated code to the following:

var types = Assembly.GetAssembly(typeof(CustomTask)).GetTypes();
var subclasses = types.Where(t => t.IsSubclassOf(typeof(CustomTask)));
var single = subclasses.Single(t => t.Name == type);
Globals.WriteLog(single.ToString());
return (CustomTask)Activator.CreateInstance(single);
share|improve this answer
add comment

After you make the changes suggested by Dan Lyons, you might also consider using fluent syntax for the query:

var type = Assembly.GetAssembly(typeof(CustomTask)).GetTypes()
                   .Where(t => t.IsSubclassOf(typeof(CustomTask)))
                   .Single(t => t.Name == type);

Or you could combine the two predicates into one:

var type = Assembly.GetAssembly(typeof(CustomTask)).GetTypes()
                   .Single(t => t.IsSubclassOf(typeof(CustomTask)) && t.Name == type);
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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