First of all you should rename your function, it doesn't perform just a search then DirSearch()
is not appropriate. What about, for example, RenameAllPngFiles()
? More context may suggest a better name, rename how? According to which rules? That should be clear.
Parameter names are usually camelCase and they're not prefixed, sDir
should then be searchFolder
or even simply path
.
You do not validate inputs. An I/O error or an invalid argument because calling logic is broken are completely different and they should be handled differently.
public static void RenameAllPngFiles(string path)
{
if (path == null)
throw new ArgumentNullException(nameof(path));
if (String.IsNullOrEmpty(path))
throw new ArgumentException("Path cannot be empty.", nameof(path));
// ...
}
Note that a null
argument and and empty argument are different (eventually path
may even be empty to use current directory) and as such they should handled separately. First test is to throw ArgumentNullException
instead of a generic ArgumentException
if path is null
. Second test may be rewritten to path.Length == 0
but I find IsNullOrEmpty()
much more explicative then I accept the price of the redundant null-checking (at least until I'll need IsNullOrWhiteSpace()
).
We're now at the core of your function. First of all drop ToList()
because you do not need it.
GetFiles()
search all files and then you can start processing. If folder is big or you will change to include sub-directories then application will lag until operation is completed, you should experiment using Path.EnumerateFiles()
which iterates file by file while search proceeds.
100 I/O parallel operations might be too much, you will find a balance only with real-world testing but a reasonable value may be around 4 (especially because each operation is very short and threading overhead may be too high). Not to mention that hardly .NET scheduler will really start so many threads for your loop then probably you will not note any difference.
Do not catch Exception
but appropriate exceptions you know may happen: IOException
and UnauthorizedAccessException
, anything else is a programming error that should be fixed during development. Also your second catch block is useless because function itself will never throw (you catch and swallow everything). Do not discard the whole batch if one operation fails.
var images = Directory.GetFiles(path, "*.png", SearchOption.AllDirectories)
.AsParallel()
.WithDegreeOfParallelism(MaxParallelIOOperations)
.ForAll(TryRenameImage);
That's all, note that you can pass function directly without wrapping inside (another) anonymous delegate. SearchOption.AllDirectories
already search inside all sub-directories. If you do not have circular hard-links then it's way faster than your recursive implementation. If you need it to handle fine granted access permissions then code will be much more complex than this and you should definitely catch relevant exceptions (Directory.EnumerateFiles()
will easily let you handle this problems case-by-case, see this SO post).
Change your Rename()
function to handle I/O errors:
private static void TryRenameImage(string path)
{
try
{
// Your logic
}
catch (IOException e)
{
// You may retry few times after a small delay,
// file might be in use elsewhere.
}
catch (UnauthorizedAccessException e)
{
// Log?
}
}
See also IOException: The process cannot access the file 'file path' because it is being used by another process.
If you write errors to console then you should use System.Console.Error
instead of System.Console
. stdout
is for program output and stderr
is for its errors.