Return to Answer

• 1

• 75.5k
• 18
• 194
• 467

var query = (from x in Directory.GetFiles(_path, "*.csv").AsParallel()
 where x.Contains(dateTime)
 select x).First();

If Directory.GetFiles(_path, "*.csv") doesn't return any files where x.Contains(dateTime), this line will blow up with an InvalidOperationException that might be a bit off-putting / surprising.

I find the query would be clearer with a 100% method syntax - this would be equivalent:

var query = Directory.GetFiles(_path, "*.csv")
 .AsParallel()
 .Where(name => name.Contains(dateTime))
 .First();

The name query is wrong/misleading. It would be a query if it were an IQueryable<string> (i.e. up to right after the .Where() call)... but it's not, var actually stands for string and what you call query is actually a fileName: .First() materializes the query, and returns the first element... or throws an exception.

It would be better to call .FirstOrDefault(), which would make your result null if no files matched your search criteria.

I don't understand why you're using var here, given that you're using explicit types everywhere else.. especially since var is just a string. Don't get me wrong though: I prefer seeing var here... and everywhere else.

#region Read CSV File

Using #region inside a method clearly says your method is doing more than one thing. Whenever you do that, you're missing a refactoring opportunity. Consider extracting a method here:

private string GetFileName(string formattedDateTime)
{
 return Directory.GetFiles(_path, "*.csv")
 .AsParallel()
 .Where(name => name.Contains(formattedDateTime))
 .FirstOrDefault();
}

(lacking context to recommend passing in an actual DateTime)

That leaves you with a method that (hopefully) now only has this #region block, which is now completely redundant and can be discarded (if there's still more to it, extract methods as needed)

That said...

FileStream stream = new FileStream(query, FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
using (StreamReader reader = new StreamReader(stream, Encoding.Default, true, 1024))

Disposing the StreamReader will also cleanly close and dispose the FileStream, but that is far from obvious unless you know it, and if Jon Skeet says it's best practice to put it in its own using block, then I can only agree with him.

Your code can use a variable (declared outside the loop's body) for the value separator: you're creating a new character array for every single row you're reading!

char[] separator = new char[] { ',' };

Or more succinctly:

var separator = new[] { ',' };

This makes the temp assignment even clearer/cleaner:

string[] temp = currentLine.Split(separator, StringSplitOptions.None);

Looking at the currentLine, it's far from obvious what's going on here:

while ((currentLine = reader.ReadLine()) != null)

Using the result of an assignment rarely results in easily readable code. I like how it cleverly combines reading the next line into currentLine and verifying whether that's a null value though... so I'm not sure I'd recommend rewriting that part after all.

It's not clear what happens to your List<string[]> when you're done reading the file, so I'll assume you're just returning it. In that case, you could consider yielding the results instead:

private IEnumerable<string[]> ReadCsv(string fileName)
{
 char[] separator = new[] { ',' };
 string currentLine;
 using (var stream = new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
 using (var reader = new StreamReader(stream, Encoding.Default, true, 1024))
 {
 while ((currentLine = reader.ReadLine()) != null)
 {
 yield return currentLine.Split(separator, StringSplitOptions.None);
 }
 }
}