patterncsharpMinor
Small method that runs various processes
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?
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:
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
Then, moving to
For the moment keep the if in-place but consider:
-
Do not perform string comparison with
-
Do not manually build paths, there is
Given this holder:
We can declare a list of known file types like this (note that his can be outside the method and static):
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
Using it I may simplify above code to:
To create the
Note that here I used a plain array but if you opt for a
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.