views:

182

answers:

3

So I'm refactoring a legacy codebase I've inherited, and in the process I found a static class that encapsulates the logic for launching 3rd party applications. It essentially looks like this (shortened for brevity to only show one application):

using System.IO;
using System.Configuration;
public static class ExternalApplications
{

   public string App1Path
   {
      get
      {
         if(null == thisApp1Path)
            thisApp1Path = Configuration.AppSettings.Get("App1Path");
         return thisApp1Path;
      }
   }
   private string thisApp1Path = null;

   public bool App1Exists() 
   {
      if(string.IsNullOrEmpty(App1Path))
         throw new ConfigurationException("App1Path not specified.");
      return File.Exists(App1Path);
   }

   public void ExecuteApp1(string args) 
   {
       // Code to launch the application.
   }

}

It's a nice attempt to separate the external applications from the rest of the code, but it occurs to me that this could have been refactored further. What I have in mind is something like this:

using System.IO;
public abstract class ExternalApplicationBase
{

   protected ExternalApplicationBase()
   {
      InitializeFromConfiguration();
   }

   public string Path { get; protected set; }

   public bool Exists() 
   {
      if(string.IsNullOrEmpty(this.Path))
         throw new ConfigurationException("Path not specified.");
      return File.Exists(this.Path);
   }

   public virtual void Execute(string args)
   {
      // Implementation to launch the application
   } 

   protected abstract InitializeFromConfiguration();

}

public class App1 : ExternalApplicationBase
{

   protected virtual void InitializeFromConfiguration()
   {
      // Implementation to initialize this application from
      // the application's configuration file.
   }

 }

 public class App2 : ExternalApplicationBase
 {

   protected virtual void InitializeFromConfiguration()
   {
      // Implementation to initialize this application from
      // the application's configuration file.
   }

 }

My concerns are as follows:

  1. A class, interface, or other construct may already exist that does this, and I just haven't stumbled across it.

  2. It may be overkill for what I want to do. Note, however, that the application uses at least three separate 3rd party applications that I have identified so far (and more are almost certain to pop up).

  3. I'm not entirely comfortable with the name of the base class. It seems fuzzy, and not very informative (but I couldn't think of much better, given that Application is already well defined, reserved by the Framework, and would create a gross level of confusion were I to use it).

  4. The idea is that I want to be able to keep the application configuration data (it's path and executable name) in the App.Config file, and check for its existence when my application starts up; when my software needs to launch the software, I want to do it through a single method call, and not have the code building command lines and trying to launch the software manually (as it currently does).

So I'm sending out a request for help, guidance, and suggestions. Anything you can profer is greatly appreciated.

P.S. I'm asking this here because I work, as I frequently do, as a sole developer at my firm; I don't have anyone else to bounce these ideas off of. You guys have tons of experience with this stuff, and it would be foolish of me not to ask for your advice, so I hope you'll all bear with me. Thanks in advance!

A: 

Seems reasonable to me.

I did something similar in the past, but I didn't have an abstract base class. Instead I passed in the application path in the constructor.

Jonathan Allen
I thought about doing that, but some of the applications we want to run synchronously, and other asynchronously. Hence the need to possibly override Execute.
Mike Hofer
If I didn't foresee a need to override Execute that way, I could get away with a single class, and just pass the information in during some sort of configuration initialization event. But this way, I get a sort of type-safe Excel or PKZip app object.
Mike Hofer
A: 

I have to agree with @Grauenwolf that it seems reasonable.

Depending on commonality you may want to provide a mechanism for encapsulating configuration retrieval (a way to pull out/set commandline args) or how to execute the application (Sync or ASync).

  1. If you find one that exists already, try putting a wrapper around it and continue.
  2. There is not too much effort in adding this extra layer and helps better separate application concerns. I feel that it is okay and can help enhance testability through mocking.
  3. AppLauncherBase or AppExecutorBase? There is no reason why the application needs to be external. Not the best names ever invented but I would try to encapsulate the purpose of the class, launching/executing applications.
  4. You may want to consider having a convention used to define configuration data/application path information in the App.Config and then implement the logic for retrieving it in the base class as the default.

Good luck and I hope this helps.

smaclell
+2  A: 

Here is another way of refactoring this:

using System.IO;
public class ExternalApplication
{
   public ExternalApplication(string path)
   {
      this.Path = path;
   }

   public string Path { get; protected set; }

   public bool Exists() 
   {
      if(string.IsNullOrEmpty(this.Path))
         throw new ConfigurationException("Path not specified.");
      return File.Exists(this.Path);
   }

   public void Execute(string args)
   {
      // Implementation to launch the application
   } 
}

public class AppFactory
{
   public ExternalApplication App1()
   {
      // Implementation to initialize this application from
      // the application's configuration file.
   }

   public ExternalApplication App2()
   {
      // Implementation to initialize this application from
      // the application's configuration file.
   }

   public ExternalApplication AppFromKey(string key)
   {
      // get from somewhere
   } 
 }

In this case, you have a single type ExternalApplication and a factory that has methods the return a properly configured application for you.

jop
I had planned on using a factory anyway; but this eliminates unnecessary derived types quite nicely. I just need to add an additional property to handle whether or not it needs to run asynchronously, and a property for its name. Thanks!
Mike Hofer
I might want to move where the ConfigurationException is thrown, though...
Mike Hofer