2

I'm working on software to play back log files. We have different types of logs from different providers.

I've created an interface that different classes will implement to play different logs. For example, ClassA will hande "somelog.txt" from ProviderA, ClassB will handle "another.log" from ProviderB, etc.

public interface ILogServer
{
    bool IsThisYourFile(string fileName, string[] content);
    // Other stuff not relevant to the question....
}

I use a factory to create the object. For each class, the factory calls IsThisYourFile() which evaluates the file name and/or the content to determine if the file is of the type it handles.

Ideally I would make IsThisYourFile static but that's not allowed. I want to use reflection to get all classes that implement ILogServer and call this function.

var types = from type in Assembly.GetExecutingAssembly().GetTypes()
                where type.GetInterface("ILogServer") != null
                select type;

foreach (var t in types)
{
    // Call the static function
    if ((bool)t.GetMethod("IsThisYourFile").Invoke(null, new object[] { fileName, content }))
    {
        server = (ILogServer)Activator.CreateInstance(t);
        break;
    }
}

This way, I only actually create one object, instead of creating an object of every class until the right one is found, leaving the others to the GC.

My current solution is to remove IsThisYourFile from the interface and just remember to implement it in each class as a static function. But that doesn't feel right.

Just wondering how others would implement this.

  • 1
    Are these objects actually expensive to create? I wouldn't expect them to be. Odds are whatever you'd try to do to avoid creating an instance would be slower than just creating an instance. – Servy Feb 6 '17 at 14:44
  • @Servy No. They are pretty simple classes. Also, this is not time-sensitive in any way. – Johnny Mopp Feb 6 '17 at 14:46
  • 5
    Premature optimization is the root of all evil. If the operation isn't expensive, and the existing implementation isn't "unacceptably slow" for your requirements, then there's no need to optimize it away. – Servy Feb 6 '17 at 14:47
  • @Servy Yes, but I am looking at this as a design issue and not an optimization issue. Creating objects until the right one is found would use very little resources. I am just curious if other design patterns exist. – Johnny Mopp Feb 6 '17 at 14:52
  • It sounds like you're trying to create a typeclass: you want a typeclass LogServerType or whatever, such that a type which implements LogServerType must have a static method called IsThisYourFile. C# doesn't have typeclasses, but you can always replace a typeclass with a generic interface. Create a generic interface ILogServerType<TLogServer> with an ordinary method IsThisYourFile. Then, where you would have had a class implementing the typeclass before, create an object implementing the interface instead. Do you get what I'm saying? I'm not explaining it very well. – Tanner Swett Feb 6 '17 at 16:33
3

It seems to me that you're attempting to replicate the mechanics already made available to you by "plain old" object-orientation, motivated by a rather misplaced assumption:

This way, I only actually create one object, instead of creating an object of every class until the right one is found, leaving the others to the GC.

If you're worrying about performance or the memory footprint instantiating all ILogServer implementations, then don't. Reflection is much costlier than a simple loop at a list of implementations, and the actual memory footprint of having the list is most certainly negligible, since they are just implementations of different strategies on how to read log files, which doesn't leave much room for carrying heavy state.

TL;DR: Instead of using reflection to find all implementations of your interface, you can just instantiate all possible implementations of ILogServer and keep them in a list, from which you can call IsThisYourFile until you find a match. Plain old OO, no complex tools involved.

  • Providing a static list requires that the types all be known at compile time. That may not be the case. If at least some of the implementations are in other assemblies that will be referencing the assembly with this class then there would be no way for this factory to do what you're proposing. – Servy Feb 6 '17 at 16:09
  • In that case I would agree, but the author doesn't mention that the classes are not known at compile time. If it is the case, I'll delete or edit my answer. – MichelHenrich Feb 6 '17 at 16:15
  • Well they're already pulling the types out of other assemblies, presumably they're doing so for a reason. – Servy Feb 6 '17 at 16:31
  • This answer doesn't really add to the OP's current solution. – John Wu Feb 6 '17 at 23:10
  • @JohnWu Why? I'm just showing a way to make it simpler, showing that tools like Reflection and such are overkill for this situation unless the implementations are not all known at design-time (which is what the question seems to point to). – MichelHenrich Feb 7 '17 at 11:31
1

Having a specialized class test random files to determine if you have a match does not seem that elegant or effective to me. I would rather create a separate class, which could be static, that knows about the different file types you support, detect the type for any given file. Then you can create the matching reader class immediately, you will not have to guess.

The detector class could return an enum which could be used in a switch to instantiate the appropriate class, passing the file path to the constructor. The enum would include a member NotRecognized. Using reflection seems pointless to me for your needs.

  • Presumably he already has such a class, and this question is asking how to implement that one class. – Servy Feb 6 '17 at 15:16
1

Call the interface member without an instance

You can include IsThisYourFile in the interface, and call it without instantiating an instance of the logger class.

Little known trick-- use an "open instance member delegate", passing null as the instance:

bool CanClassHandleFile(Type type, string path)
{
    if (type.GetInterface("ILogServer") == null) return false;

    //Get the info for the method you want to call
    var method = type.GetMethod("IsThisYourFile");

    //Create open instance member delegate
    var func = (Func<type,string,bool>)Delegate.CreateDelegate(typeof(Func<type, string, bool>), method);

    //Invoke the delegate without an instance
    return func(null, path);          
}

This is rather obscure (I can't say I recommend it honestly) but it does exactly what you want. You will get a compile-time error if you don't include IsThisYourFile in the implementation, and your program will be able to iterate over ILogServer types and check each one to see if it supports the file, without having to instantiate them.

Warning: If IsThisYourFile tries to read any non-static member variables, it will throw a null reference exception. If it tries to read this it will return null. Normally you wouldn't detect this problem until run-time, but you can mitigate the risk by following a pattern where the interface member immediately passes control to a static member, e.g.

class LogServerOne : ILogServer
{
    static private bool CheckFile(string path) //Notice this is private.  Name doesn't matter.
    {
        //Do the actual logic
    }

    public bool IsThisYourFile(string path)
    {
        return LogServerOne.CheckFile(path); //Should always be only one line of code here
    }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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