7
\$\begingroup\$

I have such problem : find all png files to disk and rename them as I did:

static void DirSearch(string sDir)
{
    try
    {
        var images = Directory.GetFiles(sDir, "*.png");
        images.ToList().AsParallel().WithDegreeOfParallelism(100).ForAll(dd =>
       Rename(dd)
   );


    }
    catch { }

    try
    {
        foreach (string path in Directory.GetDirectories(sDir))
                DirSearch(path);
    }
    catch (System.Exception excpt)
    {
        Console.WriteLine(excpt.Message);
    }

}

Actually a question : whether it is optimized enough ? Need as quickly as possible to process all png files.

\$\endgroup\$
  • 1
    \$\begingroup\$ There are 3 or 4 gazillion "rename" functions available so why not grab a couple source files and see whether you want to change anything? \$\endgroup\$ – Carl Witthoft Jul 11 '17 at 15:14
9
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your answer. If I don't have access to one of the sub folders Directory.GetFiles(path, "*.png", SearchOption.AllDirectories) - it does not give exceptions? And then the whole search is finished. Ps sorry, my English is bad but I tried \$\endgroup\$ – Lolidze Jul 11 '17 at 11:25
  • 1
    \$\begingroup\$ 100 I/O threads was the thing that jumped out at me, too. If they're all in the same directory, the rename system calls probably all need to modify the same kernel data structure (the cached copy of that directory's filenames). Probably a lot of CPU time is spent outside the actual critical section, but a number of threads equal or less than the number of physical cores is probably what you should aim for. So I agree that 4 sounds like a good suggestion. @Lolidze, you might want to try 8 to see what happens on your hardware. But startup overhead is an issue. \$\endgroup\$ – Peter Cordes Jul 11 '17 at 13:11
  • \$\begingroup\$ @Lolidze no, if you do not have access rights to one subdiirectory then it will fail. Also note that the same it's true for GetFiles(): if you do not have access to one single file then whole directory is ignored. Solution is incredibly more complicate that it should be (see stackoverflow.com/a/5098742/1207195): use it with recursion and Directory.EnumerateFiles() + Directory.EnumerateDirectories(). \$\endgroup\$ – Adriano Repetti Jul 11 '17 at 13:21
  • \$\begingroup\$ Hello, can you check whether I write link \$\endgroup\$ – Lolidze Jul 12 '17 at 12:30
  • \$\begingroup\$ Sorry, I don't understand. OK, that's original code...and? \$\endgroup\$ – Adriano Repetti Jul 12 '17 at 12:32
5
\$\begingroup\$

The indentation seems to mix spaces and tabs, sometimes even in the same line - or, at least, that's the most generous explanation. Keep it consistent.


    ... ForAll(dd => Rename(dd))

can be simplified to

    ... ForAll(Rename)

            try
            {
                foreach (string path in Directory.GetDirectories(sDir))
                        DirSearch(path);
            }
            catch (System.Exception excpt)
            {
                Console.WriteLine(excpt.Message);
            }

What exceptions are you expecting to catch here? DirSearch can't throw any, and anything which Directory.GetDirectories could throw would be better handled by input validation.


The whole recursive structure is completely unnecessary. You just need to use the appropriate overload: Directory.GetFiles(string path, string searchPattern, SearchOption searchOption).

\$\endgroup\$
  • \$\begingroup\$ Thank you for your response. If I don't have access to the folder, then GetDirectories throws an exception and all work will stop, or am I wrong ? \$\endgroup\$ – Lolidze Jul 11 '17 at 11:15

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.