Link to home
Start Free TrialLog in
Avatar of simongod
simongod

asked on

a findfiles question

i wrote a piece of code that searches for a file.  I would like someone to look through this code and see if they can see anything wrong with it.  could someone improve it for me?  What its suppose to do is search through a given directory and everything below it for a file. it really doesn't work that way right now.  so it needs improvement.

WIN32_FIND_DATA fileFindData;

CString FindFiles(CString strStartDir, CString strFileName)
{
     HANDLE hFile = FindFirstFile(strStartDir + "\\" + strFileName, &fileFindData);
     CString csFileName(fileFindData.cFileName);
     CString csSubDir(""), strFound("ERROR - file not found");
         
     if (hFile != INVALID_HANDLE_VALUE)
     {
          do
          {
               csFileName = fileFindData.cFileName;
               if ((fileFindData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) && (csFileName != ".") && (csFileName != ".."))
               {
                    csSubDir = strStartDir + "\\" + csFileName;
                    FindFiles(csSubDir);
               }

               if (csFileName.Find(".txt") != -1)
                    strFound = strStartDir + "\\" + fileFindData.cFileName;
          } while (FindNextFile(hFile, &fileFindData) && strFound == "ERROR - file not found");
          FindClose(hFile);    
     }
     return strFound;
}
Avatar of nietod
nietod

The basic idea looks fine.  

One problem I see is that you are not filtering out the two "special" directories "." and "..".   Every directory has an entry called "." that is a directory.  this entry ferers to the directory itself!.  So if you come to direcotry  "." and then seearch "." you will be searching the directory you were already searching.  And that dearch will find "." and search the directory again.  and again and again.  You will recurse forever.

Similarly all directories except for the root directory has an entry called  ".." which is for the parent directory.  if you search a directory and encouter ".." you will then search the parent directory, which you were searching earlier.  That will then lead to this directory, and then back to the parent,  then to this director and so on forever.

So you need to skipp these two directories.

Other than that it looks fine.  What is the problem you are getting?  maybe that was the problem?
opps,   No you have that in there don't you.  I missed that.


how can this be right

>>                  FindFiles(csSubDir);

The FindFiles function takes 5wo paremeters, you are passing only one.  that shouldn't even compile.

That should be

                  FindFiles(csSubDir,strFileName);

Also the loop condition

>>        } while (FindNextFile(hFile, &fileFindData) && strFound == "ERROR - file not found");

is very strange.  Using a string to control a loop like that is very unuusual and pretty innefficient.  Why are you doing that?

Avatar of simongod

ASKER

sorry it seems that i forgot to mention a few things.

the findfiles can accept 0,1or 2 arguments.  I have the function declared with the two arguments equaling NULL.  Does this help?

as for the string controling the loop.  The default for the strFound is "ERROR - file not found".  what the loop is looking for is for that to change.  if it changes then the file and its directory were found.

does that help?


The problem that i am having is when the file is found it jumps out of that while loop and returns the file name (return strFound;).  After it has completed the the next line of execution is the FindFiles(csSubDir) that is located in the if.  That seems strange and causes and assertion
The problem looks to be in your while statement.  It's going to analyze both conditions in order before evaluating the entire while().  In other words, it's going to call FindNextFile() before it evaluates strFound.  Change your while() statement to...

} while (strFound == "ERROR - file not found" && FindNextFile(hFile, &fileFindData));

... and that should do the trick.

Hope this helps!
Oops - nevermind.  I thought you had named your func FindNextFile as well.  My bad.
>> he findfiles can accept 0,1or 2 arguments.  I have the function
>> declared with the two arguments equaling
>> NULL.  Does this help?
Not at all.

I don;t see how you could expect it t work when an argument is not specified.  i..e I don't know what you want to have happen AND  I don't think I see any code that looks for this case and will make something happen in this case.

I woudl expect to see code like

if (strStartDir.length() == 0); // If no start directory, then
   strStartDir = "\\"; // use the root directory.

if (strFileName.length() == 0); // If no file name, then
   strFileName = "*.*"; // Look for any file.


Now that I think about it, the code

   FindFirstFile(strStartDir + "\\" + strFileName, &fileFindData);

is probably wrong too.  This will search that directory for any file name matching strFileName's contents.  That is fine since that is ultimately what you want. BUT it will also return only directories that match that as well.  You want to find ALL directory names and any files names that match the search skeleton.   There is no way to do that using FindFirstFile, so you have to do it manually.  You need to search the directory for *.*.  This will get your all direcotries--whic is what you want.  It also gets you all files, you will need to test each file name to see if it matches the specified search string.

OR

use two passes per directory.  First search the directory fir any files that match the search string.  Then if there is none found, search the drectory in a 2nd loop for *.* and look for entries that are sub-directories.  Then search these.    

This 2nd method is the one I usually use.

>>  if it changes then the file and its directory were found.
Why not use a boolean varaible to indicate when a match is found.  Its likely to be 100s or even 1000s of times faster.

Finally I don't see how its supposed to return information to the caller.  What is it supposed to return?  When you mke a recusrive call you ignore the return value. That is probalby wrong.

It might help if you explained what it is supposed to do.  In detail.   its very hard to understand what the goal is.
>> The problem looks to be in your while statement
It looks to me like there are little problems everywhere.  Its unclear what is a problem and what isn;t since its not clearwhat its supposed to do, but lots of things don't add up that is for sure.
ASKER CERTIFIED SOLUTION
Avatar of The Master
The Master

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial