3
\$\begingroup\$

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, "path\to\destination");
        }
        else
        {
            File.Move(fi.FullName, "path\to\errorbin");
        }
    }
}

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

\$\endgroup\$

closed as off-topic by Hosch250, Sᴀᴍ Onᴇᴌᴀ, Stephen Rauch, Malachi May 2 '18 at 15:48

This question appears to be off-topic. The users who voted to close gave these specific reasons:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Stephen Rauch, Malachi
  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Hosch250, Sᴀᴍ Onᴇᴌᴀ
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 2
    \$\begingroup\$ Can you post compilable code, please. (missing ;, split should have a capital, do is a keyword and not a valid variable name, `\` not escaped in string, etc). \$\endgroup\$ – Olivier Jacot-Descombes May 1 '18 at 20:11
  • \$\begingroup\$ I've updated my answer based on your desire to have the extension be treated as everything past the first period \$\endgroup\$ – CharlesNRice May 2 '18 at 14:02
  • \$\begingroup\$ Voted to close because this code is nowhere near compilable. \$\endgroup\$ – Hosch250 May 2 '18 at 14:34
  • \$\begingroup\$ @DainIronfootIII My apologies, new to code review should have read the help section before posting. I feel it's unfortunate that code snippets are not allowed. The entire code base is too large and includes too much sensitive data to simply share. I believe the above code (although syntax errors exist) is readable and given the answers below... answerable. If code needs to be compilable what do you guys do with interpreted languages on here? snickers \$\endgroup\$ – Christopher May 2 '18 at 18:03
  • \$\begingroup\$ You can sanitize the code within reason, but it can't be stub code. Basically, the code should be working as-is and have enough context to be reviewable. It doesn't have to be the full program, but the parts relevant to the piece should be included. \$\endgroup\$ – Hosch250 May 2 '18 at 18:15
6
\$\begingroup\$

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, @"path\to\destination");
}
else
{
    File.Move(file, @"path\to\errorbin");
}

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.

\$\endgroup\$
  • \$\begingroup\$ Finding the first '.' in a file name does not guarantee separating at the extension. Example filename: Foo.Bar.jif. Either it would need GetFileNameWithoutExtension or else searching for the last '.'. \$\endgroup\$ – Rick Davin May 2 '18 at 14:35
  • \$\begingroup\$ @RickDavin I had GetFileNameWithoutExtension first and then OP updated his answer that he wanted to count the extension as the first period so I updated my answer \$\endgroup\$ – CharlesNRice May 2 '18 at 14:43
4
\$\begingroup\$
  • 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 ? @"path\to\destination" : @"path\to\errorbin";
    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.

\$\endgroup\$
1
\$\begingroup\$

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)
{

}
\$\endgroup\$

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