Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a working version of a logfile searcher that targets a specific directory by extension type, and searches the files for certain words.

I'm wondering if my method is the most efficient implementation, or if there are things I could do better.

string startFolder = @"C:\logs\"; 

// search within these extensions
List<string> searchExtensions = new List<string>("log", "csv");

DirectoryInfo dir = new DirectoryInfo(startFolder);

// get files 
IEnumerable<FileInfo> fileList = dir.GetFiles("*.*", SearchOption.AllDirectories)
               .Where(file => searchExtensions.Contains(file.Extension.Replace(".", "")));

// strings to search for 
List<string> lSearchTerms = new List<string>() { "info", "error", "warning" };

// GetFileText simply returns the contents of the file
var queryMatchingFiles = 
                from file in fileList
                let fileText = GetFileText(file.FullName)
                where lSearchTerms.Any(val => fileText.Contains(val))
                select file.FullName;
share|improve this question

4 Answers 4

up vote 7 down vote accepted
List<string> searchExtensions = new List<string>("log", "csv");

This doesn't need to be a List<T> - you're really only using it because it's an IEnumerable<T>, so this would suffice:

var searchExtensions = new[] { "log", "csv" };

And when you're using it...

IEnumerable<FileInfo> fileList = dir.GetFiles("*.*", SearchOption.AllDirectories)
               .Where(file => searchExtensions.Contains(file.Extension.Replace(".", "")));

You're calling .Replace(".", "") to match your searchExtensions items... that replacement would be unnecessary if you had the dot in your extensions in the first place:

var searchExtensions = new[] { ".log", ".csv" };
var files = dir.GetFiles("*.*", SearchOptions.AllDirectories)
               .Where(file => searchExtensions.Contains(file.Extension));

Side note, it's often preferable to use string.Empty over "", to better communicate intent.

The name fileList is lying: it's an IEnumerable<T>, not a List<T>. Hence, files is a better, simpler name.

List<string> lSearchTerms = new List<string>() { "info", "error", "warning" };

I'm not sure why you've prefixed the searchTerms with a lowercase "L", but again, you're declaring a List<T> when all you need is an IEnumerable<T> - again, a simple array would do:

var searchTerms = new[] { "info", "warning", "error" };

Listing the log levels you're interested in, in ascending order, makes the terms seem less arbitrarily chosen. If you're using NLog for logging, Error isn't the highest log level available; are you missing "fatal"?

Also IEnumerable<T>.Contains() is case-sensitive, which means if you're searching for info and your files contain Info or INFO, you're not getting all the results you indend to retrieve. More on this here

I'm not sure why the last instruction is using var instead of explicitly declaring an IQueryable<string>, but you should probably wrap the query in parentheses and call .ToList() and declare it as an IEnumerable<string> instead.

I might have a bias against the LINQ query syntax, but I have ran some tests, and this was slightly faster than the query syntax:

var searchResults = files.Where(file => terms.Any(term => File.ReadAllText(file.FullName).Contains(term)))
                         .Select(file => file.FullName);

Side note about comments: there's too many of them. They are redundant and often only rephrase what the code is already saying; comments should say why, not what.

// filter doesn't support regex, so get all files *and then* filter by extension:
var files = dir.GetFiles("*.*", SearchOptions.AllDirectories)
               .Where(file => searchExtensions.Contains(file.Extension));
share|improve this answer
    
Saying something is slow over 100,000 iterations is not worth mentioning. This is code review and without the entire application being profiled, what is it slow compared to? Avoid premature optimization and optimize based on slowdowns across the application. You're talking about nano second differences here. I'm going to take a stab in the dark and say that you did these tests in Debug mode as well yes? –  Vince Panuccio Jun 12 at 4:22
    
@VincePanuccio They were. I was just curious to see if there was any kind of a difference - I'm not saying query syntax is slow, I'm just saying the syntax I prefer seems to be faster. FWIW I edited that part out. –  Mat's Mug Jun 12 at 4:25
1  
Thanks, a lot of good stuff here. One note: I'm pulling the extensions list out of config, and I don't want to rely on someone else remembering to put the dot in. So I'm keeping it as a replace. –  adamcodes Jun 12 at 17:04

First of all I'll suggest to fix (or make explicit) the behavior if the extension is uppercase: important.CSV will not be considered by this software. At least a comment in the code.

Now about the performance:

 where lSearchTerms.Any(val => fileText.Contains(val))

in this way for every file that doesn't match you do 3 full scan of the file content, 2+1/2 if contains only "warning", 1+1/2 if contains only "error" and 1/2 if contains "info". Also if "Contains" implements the best possible string search algorithm (not sure), it's not the best algorithm for this use (search using a finite set of patterns)

Some algorithms that solve optimally this problem are

  1. Rabin–Karp
  2. Commentz-Walter
  3. Rabin–Karp

More info here

If you want to avoid this solution I suggest to use a standard regex: at least it will make the search only one time (and a fixed pattern search can be modeled quite easily with an automa)

share|improve this answer

There is one thing you could have done more efficient. When you read the files you wrote [star.star] which means that all files, regardless of type will get returned. This might be insufficient if there are many different files that are not related to your program in the target directory

share|improve this answer
    
True, but the file list will be filtered by 1+ extensions (I wanted to keep the question simple, so I used a static list). I figured since the extensions list will be variable I would grab all and then filter. There's probably a way to pass in the desired extensions, so I'll check that out next. Thanks. –  adamcodes Jun 12 at 17:00

@AdamCodes - Based on all your responses so far above, here are my comments and suggestions:

  1. Remember to remove only the first dot from the extension. We have multi-dotted extensions (like ".aspx.cs"). Your above code will make these files "aspxcs" which will not match anything.

  2. When you load the extensions array/list/collection, make two copies of every extension configured. One uppercase, one lower. Always remove the "." if prefixed with one.

  3. Contains() is case sensitive and also slow. Instead, use RegEx to perform the search. Yes, your code will grow a little more, but will be more efficient.

RE: "var", I am not a supporter of using "var" and prefer strong-typing my variables.

share|improve this answer
    
var variables are still strongly-typed. It is dynamic variables that are not strongly typed. –  ANeves Jun 19 at 10:48
    
Well, you are still letting the compiler decide what type it should be. And it infers that by looking at the potential values you want to assign. There are places where this is disastrous for your program -- I have had a few such blowups. So I personally prefer not to use them. Using VAR is anyway a matter of preference than something "enforced". So let's agree to disagree on its usefulness :) –  Sujay Sarma Jun 19 at 13:33
    
I expressed no opinion, so we can't disagree. ;) I insist: the type of var is deterministic, it is the exact "return type" of the expression that is assigned to it; the compiler has nothing to decide. -1 because I believe that your answer's last sentence is incorrect, and I will remove my down-vote if you decide to change it. (No offence meant! Thank you for being part of the community and actively participating.) –  ANeves Jun 19 at 17:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.