trjhtr

My program inspects filenames which are delimited via underscores. So an example may be:

This_is_an_example_of_my_filename.tif

A counter example may be:

This_is_a_very_bad_document.tif

The filename should always consist of 7 distinct values within the filename. Those values are used to create an instance of a class object which allows me to further validate data based on segment location in the filename.

The program should only consider *.tif files in the directory, and ignore all others.

If the filename is invalid (too few/too many fields, or field data is improperly formatted) we move it to an error bin. If it is good, I upload the data to a database and move the file on it's merry way.

The code that pulls these filenames apart looks like:

foreach(string file in Directory.GetFiles(path))

 FileInfo fi = new FileInfo(file)
 if(fi.Extension == ".tif")
 
 MyDocumentObject do = new MyDocumentObject();
 string filename = fi.Name;
 string filenameAsArray = filename.split('_');
 string lastIndexNoExtension = (filenameAsArray[filnameAsArray.Length -1].Split('.'))[0];
 filenameAsArray.SetValue(lastIndexNoExtension, (filenameAsArray.Length - 1)); 
 if(filenameAsArray.Length == 6)
 
 for(int x=0;x<=(filenameAsArray.Length - 1);x++)
 
 switch(x)
 
 case 0:
 string part1 = filenameAsArray[x];
 do.part1 = part1;
 break;

 //so on for 1-6
 
 
 
 bool validationResults = do.ValidateFields();
 if(validationResults)
 
 File.Move(fi.FullName, "pathtodestination");
 
 else
 
 File.Move(fi.FullName, "pathtoerrorbin");
 
 

My main point of focus for this review is on the method of pulling apart the filename into an array of text segments and then assigning that data to an instance object and operating on it.

In practice I'm pretty happy - it does the job and is zippy about it. But looking at it... just bugs me.

EDIT: I forgot to mention the reason I use my method of splitting to remove the file extension is due to issues that arise when a file in the directory is named something like:

this_is_a_trick.txt.tif

First since you only care about tif extensions just pass that into the Directory.GetFiles

foreach (string file in Directory.GetFiles(path, "*.tif"))

No need for an if statement checking extension

Also you don't need to create a FileInfo object to get the name of the file use

string filename = Path.GetFileName(file);
filename = filename.Substring(0, filename.IndexOf('.'));

returns you the file name without the extension so you don't need to do the array manipulation.

If me since there is only 6 values I wouldn't do a loop statement. You need to code each switch and set each property so the loop isn't really helping you save code nor time. Just set them. if they need to change in the future it's less code to change and the loop and switch would need to change as well.

string filename = Path.GetFileName(file);
filename = filename.Substring(0, filename.IndexOf('.'));
string filenameAsArray = filename.Split('_');
if (filenameAsArray.Length == 6)

 document.part1 = filenameAsArray[0];
 document.part2 = filenameAsArray[1];
 document.part3 = filenameAsArray[2];
 document.part4 = filenameAsArray[3];
 document.part5 = filenameAsArray[4];
 document.part6 = filenameAsArray[5];

bool validationResults = document.ValidateFields();
if (validationResults)

 File.Move(file, @"pathtodestination");

else

 File.Move(file, @"pathtoerrorbin");

I also changed your object from do to document because do to most English speakers is a verb and they not going to think it stands for document object. Again for years later whoever looks at this to make it easier to understand.

• 
  

  You can filter files in GetFiles with an additional search pattern argument

  
  
  

  Directory.GetFiles(path, "*.tif")
  

  
  


• 
  

  You can retrieve the filename without extension with

  
  
  

  string filename = Path.GetFileNameWithoutExtension(file);
  

  
  



• do is a C# keyword, use doc as variable name instead. (Your code doesn't compile with do as identifier. You could escape it by prefixing it with an at-sign @do, but I prefer a regular name.




• Move MyDocumentObject to the scope where it is needed.




• The standard way of iterating is for(int i = 0; i < N; i++) and not i <= N-1. (Note: I've eliminated the loop anyway.)




• The inner for-loop and the switch-statement are over complicated. Assign the array elements directly.




• Use better naming.




• No need to validate the document object if the number of parts is not 6.




• Don't call File.Move at two different places.

foreach (string path in Directory.GetFiles(directory, "*.tif")) 
 string filename = Path.GetFileNameWithoutExtension(path);
 string parts = filename.Split('_');
 bool isFilenameValid = false;
 if (parts.Length == 6) 
 var doc = new MyDocumentObject 
 part1 = parts[0],
 part2 = parts[1],
 // and so on for part3 - part6
 ;
 isFilenameValid = doc.ValidateFields();
 
 string destination = isFilenameValid ? @"pathtodestination" : @"pathtoerrorbin";
 File.Move(path, destination);

You end up in having 1 loop and 1 if, instead of 2 loops 3 ifs and one switch. Reducing the number of flow control statements from 6 to 2 enhances the readability.

Minor but Directory.EnumerateFiles will be a little faster and use less memory as it does not download all the filenames before starting.

Other than that go with the accepted answer from CharlesNRice

var txtFiles = Directory.EnumerateFiles(path, "*.txt");
foreach (string currentFile in txtFiles)