1

I am trying to delete all pdf files older than 1 hour, I am using this code

System.IO.DirectoryInfo directory = new DirectoryInfo(System.Configuration.ConfigurationManager.AppSettings["PDFFilesPath"]);
var files = directory.GetFiles("*.pdf").Where(f => DateTime.Now.Subtract(f.CreationTime).TotalMinutes >= int.Parse(System.Configuration.ConfigurationManager.AppSettings["MinutesOld"]));
foreach (var file in files)
{
    file.Delete();
}

The code is working fine and as expected.

But I am wondering if there is any way to not use foreach or any other smaller approach to achieve this

Thanks in advance!

Pawan Nogariya
  • 8,330
  • 12
  • 52
  • 105
  • What's wrong with `foreach` ? Your approach looks fine to me. – Bridge Jan 31 '13 at 09:48
  • You can't delete them all at once, if that's what you mean. You could use `Parallel.ForEach` to run them in parallel though (.Net 4 and 4.5) – stuartd Jan 31 '13 at 09:49
  • using foreach is the best way or i should say the simplest! :-) – Srinivas Jan 31 '13 at 09:49
  • Run this code on the background thread would be better if you delete many files – cuongle Jan 31 '13 at 09:50
  • 1
    The `foreach` **is** the smaller part of this, I'd say... =) But, to shorten your code you could shorten the qualified references by putting your namespaces in `using` statements at the top. – J. Steen Jan 31 '13 at 09:53
  • Define _smaller approach_. In terms of eficiency `foreach` is the most effiicent approach since it doesn't create something new(like a `List`). – Tim Schmelter Jan 31 '13 at 09:55
  • @TimSchmelter - From smaller approach I mean anything in my code that can be achieved better in the sense of lesser code and performance. I was afraid if I am doing it correctly to process and delete one file at a time, so something like deleting all the files at once etc. However reading all the comments I think the approach is okay. – Pawan Nogariya Jan 31 '13 at 10:03
  • @PawanNogariya: Even if there would be something which appears to delete all at once in a single line, it would use a loop implicitely. In terms of simplicity and readability i also cannot imagine something better than your `foreach`. – Tim Schmelter Jan 31 '13 at 10:07
  • @TimSchmelter - Great then! I will stick with my code now. Actually my main concern was to know if the code is good enough from performance perspective. – Pawan Nogariya Jan 31 '13 at 10:14
  • 1
    @PawanNogariya: The only thing i see at the first sight is that you could replace `directory.GetFiles` with `directory.EnumerateFiles`. The latter(new in 4) does't need to load all into memory before it starts processing. – Tim Schmelter Jan 31 '13 at 10:18
  • @TimSchmelter - Great! Thanks Tim! Replaced it with `EnumerateFiles`. So you mean it will not load the files with EnumerateFiles until it filter's the files with creationdate? BTW I have changed my code to this now directory.EnumerateFiles("*.pdf").Where(f => DateTime.Now.Subtract(f.CreationTime).TotalMinutes >= int.Parse(System.Configuration.ConfigurationManager.AppSettings["MinutesOld"])).ToList().ForEach(f => f.Delete()); – Pawan Nogariya Jan 31 '13 at 10:26
  • 1
    Try [Codereview.stackexchange](http://codereview.stackexchange.com/) if you want to review or enhance working code – DarknessBeginsHere Jan 31 '13 at 23:43
  • @Dark.Amer - +1 Right info :) – Pawan Nogariya Feb 01 '13 at 06:21

2 Answers2

0

You might use LINQ adding the below at the end of your Linq code.

.ToList()
    .ForEach(f => f.Delete());

Bear in mind that even Linq behind the scene will use an iterator; I would leave as it is then

have a look at this answer as well

Delete files older than 3 months old in a directory using .NET

Community
  • 1
  • 1
Massimiliano Peluso
  • 26,379
  • 6
  • 61
  • 70
0

If you want a one liner you could use:

files.ToList<FileInfo>().ForEach((FileInfo f) => { f.Delete(); });

But this is inefficient and there really isn't any point to it. You're doing it the right way at the moment.

See this article for more info.

Cashley
  • 516
  • 5
  • 16