HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Small method that runs various processes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
methodthatsmallprocessesrunsvarious

Problem

I have a project which runs from a db table - it's purpose is to run processes written in various ways.

Currently it deals with python scripts, vbs scripts and console exe files.

It works but I'm wondering if there any obvious refactors?

private Process aProcess;

public void runProcess(string aPath,string aName,string aFiletype)
{

  aProcess = new Process();
  string stInfoFileName = null;
  string stInfoArgs = null;
  bool blUseShell = false;

  //if it is to call a python script
  if(aFiletype == "py")
  {
    stInfoFileName = @"python.exe";
    stInfoArgs = string.Format("\"{0}{1}{2}\"",aPath,Path.DirectorySeparatorChar,aName);
  }

  //if it is to call console app exe
  if(aFiletype == "exe")
  {
    stInfoFileName = aName;
    stInfoArgs = string.Format("\"{0}{1}{2}\"",aPath,Path.DirectorySeparatorChar,aName);
    blUseShell = true;
  }

  //if it is to run vbs script
  if(aFiletype == "vbs")
  {
    stInfoFileName = "cscript";
    stInfoArgs = string.Format("/B \"{0}{1}\"",@aPath,@aName);
  }

  aProcess.StartInfo.FileName = stInfoFileName;
  aProcess.StartInfo.Arguments = stInfoArgs;

  aProcess.StartInfo.WorkingDirectory = aPath;           //<< does not seem to need the extra quotes

  aProcess.StartInfo.UseShellExecute = blUseShell;
  aProcess.StartInfo.RedirectStandardOutput = false;

  aProcess.Start();

  this.aProcess.WaitForExit();
  this.aProcess.Dispose();

}

Solution

I'd first start with naming conventions. Prefixes are usually avoided and method name is PascalCase:

public void RunProcess(string path, string fileName, string fileExtension)


Honestly I'd accept just one parameter with full path and I'd split it inside this method but you might not be able to change this then I won't comment about it.

First I'd extract a method to create the StartupInfo leavinig in this method only invocation and parameters validation:

if (path == null)
    throw new ArgumentNullException(nameof(path));

// More validations...

using (var process = Process.Start(CreateStartupInfo(path, fileName, fileExtension)))
{
    process.WaitForExit();
}


Then, moving to CreateStartupInfo() implementation, I'd first drop those if in favor of a Dictionary or a plain list because it's much easier to extend and it might come from a configuration file (no need to change code to support more file types.)

For the moment keep the if in-place but consider:

-
Do not perform string comparison with ==. It's a current culture aware case sensitive comparison, it's not obvious and it's not probably what you want. Use String.Equals() specifying the right StringComparer/StringComparison value.

-
Do not manually build paths, there is Path.Combine() for that. Let's see an example where for simplicity I have as input the full file path.

Given this holder:

sealed class KnownFileType
{
    public KnownFileType(string type,
        bool useShellExecute,
        Func interpreter,
        Func interpreterCommandLine)
    {
        // ...
    }

    public string Type { get; }
    public Func Interpreter { get; }
    public Func InterpreterCommandLine { get; }
    public bool UseShellExecute { get;  }
}


We can declare a list of known file types like this (note that his can be outside the method and static):

var fileTypes = new KnownFileType[]
{
    new KnownFileType(".py", false, x => "python.exe", x => $"\"{x}\""),
    new KnownFileType(".exe", true, x => Path.GetFileName(x), x => $"\"{x}\""),
    new KnownFileType(".vbs", false, x => "cscript.exe", x => $"/B \"{x}\""),
};


Do you need to add new known file type? Add one line here. If those values come from a configuration file then you might want to use String.Format() instead (be careful if configuration can be tampered.) Because I dislike escape sequences I'd introduce an helper method:

private static string Quote(string text) => '"' + text + '"';


Using it I may simplify above code to:

new KnownFileType(".py", false, x => "python.exe", x => Quote(x))


To create the ProcessStartupInfo you just need to:

string extension = Path.GetExtension(fullPath);
var fileType = fileTypes.First(x => String.Equals(x.Type, extension, StringComparison.InvariantCultureIgnoreCase));

return new ProcessStartInfo
{
    UseShellExecute = fileType.UseShellExecute,
    FileName = fileType.Interpreter(fullPath),
    Arguments = fileType.InterpreterCommandLine(fullPath),
    WorkingDirectory = Path.GetDirectoryName(fullPath)
};


Note that here I used a plain array but if you opt for a Dictionary (with the proper case insensitive comparer for keys) then code may be slightly simplified to:

var fileType = fileTypes[Path.GetExtension(fullPath)];
// ...

Code Snippets

public void RunProcess(string path, string fileName, string fileExtension)
if (path == null)
    throw new ArgumentNullException(nameof(path));

// More validations...

using (var process = Process.Start(CreateStartupInfo(path, fileName, fileExtension)))
{
    process.WaitForExit();
}
sealed class KnownFileType
{
    public KnownFileType(string type,
        bool useShellExecute,
        Func<string, string> interpreter,
        Func<string, string> interpreterCommandLine)
    {
        // ...
    }

    public string Type { get; }
    public Func<string, string> Interpreter { get; }
    public Func<string, string> InterpreterCommandLine { get; }
    public bool UseShellExecute { get;  }
}
var fileTypes = new KnownFileType[]
{
    new KnownFileType(".py", false, x => "python.exe", x => $"\"{x}\""),
    new KnownFileType(".exe", true, x => Path.GetFileName(x), x => $"\"{x}\""),
    new KnownFileType(".vbs", false, x => "cscript.exe", x => $"/B \"{x}\""),
};
private static string Quote(string text) => '"' + text + '"';

Context

StackExchange Code Review Q#154989, answer score: 4

Revisions (0)

No revisions yet.