trjhtr

This is legacy, so I'm not really looking for improvements on architectural approach.

Here's what the method does:

1. Extracts filename from a string containing full file path (passed in as parameter)


2. Checks to see if the filename extracted is not null or empty or just white spaces


3. 
   

   If check in step 2 passes, then it uses naming conventions in the filename, to determine the logical "Type" for the file as follows:

   
   
   
   1. If filename contains __Import__, then setType to Source Data
      
   
   
   2. If filename contains __Export__, then setType to Destination Data
      
   
   
   3. If filename contains __Transform__, then setType to Transformational Data
      
   
   
   4. If none of the conditions from 3.1 through 3.3 are met, then default the "Type" to General.
   
   

Is there a way to make the if...else if... else if... block more concise, and elegant without loss of functionality?

private static string GetFileDataType(string fullFileNameWithPath)

 // extract filename only
 var fileName = Path.GetFileNameWithoutExtension(fullFileNameWithPath);
 var fileDataType = string.Empty;

 // if filename is not empty
 if (!string.IsNullOrWhiteSpace(fileName))
 
 // if... else... to determine type of file contents based on file name convention
 // legacy code, so not looking for architectural improvements etc... :)
 if (fileName.Contains("__Import__"))
 
 fileDataType = "Source Data";
 
 else if (fileName.Contains("__Export__"))
 
 fileDataType = "Destination Data";
 
 else if (fileName.Contains("__Transform__"))
 
 fileDataType = "Transformational Data";
 
 else
 
 fileDataType = "General";
 
 

 return fileDataType;

I find the cleanest way is to replace the ugly if/else if/else with a dictionary:

private static IDictionary<string, string> FileDataTypes = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)

 ["Import"] = "Source Data",
 // ..

The also not so pretty Contains can be replaced with a nice regex that will grow or shirnk automatically if you add/remove mappings:

private static string GetFileDataType(string fullFileNameWithPath)

 var fileName = Path.GetFileNameWithoutExtension(fullFileNameWithPath);
 var fileContentsMatch = Regex.Match(fileName, $"__(?<Type>", FileDataTypes.Keys))__", RegexOptions.ExplicitCapture 

Notice that both the dictionary and the regex are case insensitive. You should always make paths case insensitive in Windows. You are lucky that this worked for so long.

(Dislaimer: It's just an example so I was to lazy to implement all the empty/null checks)

You're searching through the file name for each type until you find one. If you use the __ as delimiters and find the indexes of the string between them, you will only search the whole string once.

By extracting the substring with the type, now you can use a switch block to return the appropriate value:

private static string GetFileDataType(string fullFileNameWithPath)

 const string Delimiter = "__";
 // extract filename only
 var fileName = Path.GetFileNameWithoutExtension(fullFileNameWithPath);
 var fileDataType = string.Empty;
 int indexA = fileName.IndexOf(Delimiter);
 string type = "";
 if(indexA != -1 && indexA != 0)
 
 indexA += 2;
 int indexB = fileName.IndexOf(Delimiter, indexA);
 if(indexB != -1)
 
 type = fileName.Substring(indexA, indexB - indexA);
 
 
 switch(type)
 
 case "Import":
 return "Source Data";
 case "Export":
 return "Destination Data";
 case "Transform":
 return "Transformational Data";
 default:
 return "General";
 

If the string is empty or malformed "General" will be returned. Because the check for this is in one place it can easily be changed, if desired.

The method you have provided looks clean, is short and is easy to understand. It does what it should do but calls Contains() in the worst case 3 times which could be avoided.

A filedatatype other than General can only happen if at least the delimiter __ is found twice. So why don't we Split() the filname and if we get an array which length is at least 3 we have something to work with.

Let us assume the filename without extension reads SomeFile__Export__SomeMoreText if we call Split() using __ as splitting argument we would get "SomeFile", "Export", "SomeMoreText" now we can easily iterate over the array starting at the second element and ending at the element before last and using a switch on each element to gain the desired filedatatype.

Overall this would look like

private static string GetFileDataType(string fullFileNameWithPath)


 var fileName = Path.GetFileNameWithoutExtension(fullFileNameWithPath);

 if (string.IsNullOrWhiteSpace(fileName)) return string.Empty; 

 var possibleFileDataTypes = fileName.Split(new string "__" , StringSplitOptions.None);

 if (possibleFileDataTypes.Length < 3) return "General"; 

 for (var i = 1; i < possibleFileDataTypes.Length - 1; i++)
 
 switch (possibleFileDataTypes[i])
 
 case "Import":
 return "Source Data";

 case "Export":
 return "Destination Data";

 case "Transform":
 return "Transformational Data";
 
 
 return "General";

and produces the following results

GetFileDataType(string.Empty) -> string.Emtpty
GetFileDataType(null) -> string.Emtpty
GetFileDataType(@"C:folderText__Export__SomeOtherText.txt") -> "Destination Data"
GetFileDataType(@"C:folderText__SomeOtherText__SomeMoreText.txt") -> "General"
GetFileDataType(@"C:folderSome__File__Import__Some__More__Text.txt") -> "Source Data"
GetFileDataType(@"C:folder__Transform__Some__More__Text.txt") -> "Transformational Data"
GetFileDataType(@"C:folder__Export__.txt") -> "Destination Data"
GetFileDataType(@"C:folder__Export.txt") -> "General"
GetFileDataType(@"C:folder____.txt") -> "General"
GetFileDataType(@"C:folder____.txt") -> "General"

A different approach based on the idea to use something like a dictonary (t3chb0t's answer) is using two arrays of strings internally. One to check if this "string-component" is present in the file name, the other one to return the file type.

Using these two you need to iterate the first array and return from the second array using same index if you find something.

This removes the if/else structure and seems to be easy to read to me, eventough (or maybe because) it is not as advanced as the other answers and keeps the contains().

private static string GetFileDataType(string fullFileNameWithPath)
 
 // extract filename only
 var fileName = Path.GetFileNameWithoutExtension(fullFileNameWithPath);

 if (string.IsNullOrWhiteSpace(fileName))
 return string.Empty;

 String fileNameComponents = "__Import__", "__Export__", "__Transform__" ;
 String fileTypes = "Source Data", "Destination Data", "Transformational Data" ; 

 for (int i = 0; i < fileNameComponents.Length; i++)
 
 if (fileName.Contains(fileNameComponents[i]))
 return fileTypes[i];
 

 return "General";
 

This approach only aims at the core of the question (removing the if/else) whitout considering any improvments to performance.

Composing with a list of functions, one can drastically reduce the number of needed lines:

private static string GetFileDataType(string fullFileNameWithPath)

 var fileName = Path.GetFileNameWithoutExtension(fullFileNameWithPath);
 return matchingRules.First(tuple => tuple.Item1(fileName)).Item2;

With matchingRules declared as follow:

var matchingRules = new List<(Func<string, bool>, string)>();
matchingRules.Add((f => !string.IsNullOrWhiteSpace(f) && f.Contains("__Import__"), "Source Data"));
matchingRules.Add((f => !string.IsNullOrWhiteSpace(f) && f.Contains("__Export__"), "Destination Data"));
matchingRules.Add((f => !string.IsNullOrWhiteSpace(f) && f.Contains("__Transform__"), "Transformational Data"));
matchingRules.Add((f => !string.IsNullOrWhiteSpace(f), "General"));
matchingRules.Add((f => true, string.Empty));