5
\$\begingroup\$

I have a Windows program that prompts the user to input a file path and filename. I then check that the file exists with Directory.Exists. Then the user inputs a filename and I check it using this answer to check whether the filename is valid or not (it may or may not yet exist).

using System;
using System.IO;
using System.Text.RegularExpressions;

public static string GetFilePath()
{
    string filePath = " ";
    bool directoryFound = true;
    do
    {
        if (!directoryFound)
        {
            Console.WriteLine("This directory not found:\n{0}\nPlease double check file path. \nPlease Double check that this program has read and write access to the directory.\n", filePath);
        }

        Console.WriteLine("Please specify file location:");
        filePath = @Console.ReadLine(); 
        Console.Clear();

        directoryFound = Directory.Exists(filePath);
        if (filePath.Length >= 240) //This looks like a bad solution
        {
            directoryFound = false;
            Console.WriteLine("Please keep the filepath under 240 chars so that you still are able to provide a name for the file.");
        }

    } while (!directoryFound);

    filePath = GetFileName(filePath);

    return filePath ;
}

    public static string GetFileName(string filePath)
    {
        string fileName = " ";
        bool isValidName = true;
        int fileNameMaxLength = 0;
        do
        {
            if (!isValidName)
            {

                Console.WriteLine("{0} \nis not a valid filename. Make sure you enter a valid filename.\nYour max filename length should be {1}", fileName, fileNameMaxLength);
            }

            Console.WriteLine("Please enter the name of the file:");
            fileName = Console.ReadLine();
            Console.Clear();

            string illegalChars = @"^(?!^(PRN|AUX|CLOCK\$|NUL|CON|COM\d|LPT\d|\..*)(\..+)?$)[^\x00-\x1f\\?*:\"";|/]+$";

            isValidName = Regex.IsMatch(fileName, illegalChars, RegexOptions.CultureInvariant);

            if (fileName.Length + filePath.Length > 254) //254 instead of 260 in order to account for file extension
            {
                isValidName = false;
                fileNameMaxLength = 254 - filePath.Length;
            }

        } while (!isValidName);
        return Path.Combine(filePath, (fileName + ".ppm"));
    }

}

How could I make this be more elegant? I especially don't like:

if(filePath.Length >= 240)

I need to make sure that:

  • The filepath exists
  • The filename is valid
  • That filename + filepath <= 260 (this is max char limit on Windows)
  • That my program doesn't crash while doing this
\$\endgroup\$
8
\$\begingroup\$

For the if(filePath.Length >= 240) part, you can use the Path object. It throws a PathTooLongException.

You can then change to this :

try
{
    filePath = Path.GetFullPath(filePath);
}
catch (PathTooLongException ex)
{
    directoryFound = false;
    Console.WriteLine("Please keep the filepath under 240 chars so that you still are able to provide a name for the file.");
}

Note : you can find references to Path object here and see all exception you have to handle invalid path input.

Now for GetFileName(), you can use the same object Path.GetFileName(). It will check if the name is valid.

ArgumentException : path contains one or more of the invalid characters defined in GetInvalidPathChars.

...
fileName = Console.ReadLine();
            Console.Clear();
try
{
    fileName = Path.GetFileName(fileName);
}
catch (ArgumentException ex)
{
    // Whatever exception handling you want.
}

Note : Since you have to check more than one time if the Path is valid, I would create a method to do this.

public static bool PathIsValid(string inputPath)
{
    try
    {
        Path.GetFullPath(inputPath);
    }
    catch(PathTooLongException ex)
    {
        return false;
    }
    return true;
}
\$\endgroup\$
  • \$\begingroup\$ I like this, thanks. It's frowned upon to use exceptions for validation, but in this case it's about the only certain way to determine path validity. Besides, how often does any code have to check a file path? In well written code, it should be a rare task, normally. \$\endgroup\$ – ProfK Jan 14 '17 at 6:03
-2
\$\begingroup\$

As you may notice in the code below, I tried to avoid Try{}Catch{} statements. Look, it is not as difficult as that for someone to understand the skeptical, if he/she follows the program 'Logically' .

Now, the comments help to clarification. Some comments however help to show that I took under account some conditions, but they finally went unnecessary because other parts of the program INDIRECTLY cover the case. For example, around lines 322 - 325, the if (fileNameSubStr.Length < 5).... hosts the comment //OPTIMIZATION: This block cannot run, which indeed means that this block will not run, since other previous checks INDIRECTLY (by accident if you like!!!) cover this if case. Why I did not delete the unnecessary if statements?? Exactly for helping the programmer to understand what is going on and what were my thoughts during writing the code.

using System;
using System.Text;
using System.IO;
using System.Runtime.InteropServices;
using System.Text.RegularExpressions;

class Program
{
[DllImport("msvcrt.dll")]
static extern bool system(string str);

public static bool ControlFullPathNameValidation(string fullPathFileName)
{
   int counter =        
   int position = 0;
   char slash = '\\';
   //char[] getInvalidPathChars = Path.GetInvalidPathChars();//I could not make it work
   char[] getInvalidPathChars = { '<', '>', ':', ',', '"', '/', '|', '?', '*', '.', '[', ']', '=' };

   string[] getLogicalDrives = Directory.GetLogicalDrives();
   string directoryPathSubStr = string.Empty, fileNameSubStr = string.Empty;
   string yn = string.Empty;
   bool tf = true;

   Regex fileNameSubStrRegexControl = new Regex(@"^[a-zA-Z]+$");

   Console.ForegroundColor = ConsoleColor.DarkRed;
   //Console.Write("Undesired path name characters: ");
   //foreach (var item in getInvalidPathChars)
   // Console.Write(item + " ");
   //Console.WriteLine();

   while (true)
   {
      //Primary checks
      if (fullPathFileName.Length < 8)
      {
      //1st Test Passed
      Console.Write("Invalid full name: The full file path name typed\n" +
      "contains less than eight characters. Retry (y/n)? ");
      fullPathFileName = string.Empty;
      tf = false;
      break;
   }
   else
   {
      for (counter = 0; counter < getLogicalDrives.Length; ++counter)
      if (fullPathFileName.Remove(1) == getLogicalDrives[counter].Remove(1))
      {
         //2nd Test passed.
         //Console.WriteLine("Hosting Logical Drive: {0}", getLogicalDrives[counter]);
         counter = 0;
         break;
      }
      if (counter != 0)
      {
         Console.Write("Error: The logical directory provided corresponds to\n" +
   "no physical disk drive of this machine. Retry (y/n)? ");
         fullPathFileName = string.Empty;
         tf = false;
         break;
      }
}
if (!fullPathFileName.Contains(@"\"))
{
   Console.Write("Invalid name: Full name contains no slash at all. Retry (y/n)? ");
   fullPathFileName = string.Empty;
   tf = false;
   break;
}
else if (fullPathFileName.Substring(1, 2) != @":\")
{
   Console.Write("Invalid name: The second or/and third characters of the\n" +
"full file path are not of \":\\\" format. Retry (y/n)? ");
   fullPathFileName = string.Empty;
   tf = false;
   break;
}
else if (fullPathFileName.Substring(4, 1) == @"\")
{
   Console.WriteLine("*Invalid name: Slash found right\n" +
"after slash. Retry (y/n)? ");
   fullPathFileName = string.Empty;
   tf = false;
   break;
}

//Let us break the fullPathFileName into two substrings
directoryPathSubStr = fullPathFileName.Remove(fullPathFileName.LastIndexOf(@"\")) + "\\";//Directory path
fileNameSubStr = fullPathFileName.Remove(0, fullPathFileName.LastIndexOf(@"\") + 1);//File name 
fullPathFileName = string.Empty;//How do we kill this variable? It is not useful anymore.
if(fileNameSubStr.Length > 250) {Console.WriteLine("Error: File Name larger than 250 characters. Please, retype file name!"); break;}

//Let us examine the validity of the 1st substring (Directory path) 
position = 2;

while (directoryPathSubStr.IndexOf(slash, position + 1) != -1)
{
/*Sorry. I cannot optimize this while_block any further.
I think that it is susceptible of more optimization. 
In the future, I will try to start the block with 
while(subStr1.IndexOf(slash, position + 1) + 1 < subStr1.Length) */

if (directoryPathSubStr.Substring(directoryPathSubStr.IndexOf(slash, position + 1) - 1, 1) == @"\")
{
Console.Write("Invalid name: Slash found right\n" +
"after slash. Retry (y/n)? ");
tf = false;
break;
}

string currentSubDirectory = directoryPathSubStr.Substring(position + 1, directoryPathSubStr.IndexOf(slash, position + 1) - position - 1);
// foreach (var item in getInvalidPathChars) //Not quite proper. I could not find any foreach inside_counter
for (int i = 0; i < getInvalidPathChars.Length; ++i)
{
if (directoryPathSubStr.Substring(position + 1, directoryPathSubStr.IndexOf(slash, position + 1)
- position - 1).Contains(getInvalidPathChars[i].ToString()))
{
Console.Write("Invalid name: Invalid character <{0}> in subdirectory\nstring '{1}'. Retry (y/n)? ", getInvalidPathChars[i],
currentSubDirectory, position + directoryPathSubStr.Substring(position + 1, directoryPathSubStr.IndexOf(slash, position + 1)
- position - 1).IndexOf(getInvalidPathChars[i].ToString()) + 1);

tf = false;
break;
}
}
if (!tf)
break;

position = directoryPathSubStr.IndexOf(slash, position + 1);
}

if (!tf)
break;
//Now, let us examine the validity of the 2nd substring 
/*if (fileNameSubStr.Length < 5)
{
//OPTIMIZATION: This block cannot run
Console.Write("Invalid File Name: the name contains less\n" +
"than five characters. Retry (y/n)? ");
tf = false;
break;
}
else*/
if (!fileNameSubStr.Contains("."))
{
//Check if the full file name contains no punctuation
Console.Write("Invalid File Name: No punctuation in\n" +
"file extension name. Retry (y/n)? ");
tf = false;
break;
}
else if (fileNameSubStr.IndexOf(".") != fileNameSubStr.Length - 4)
{
//Check if the punctuation is exactly where it is supposed to be (4 positions before end)
Console.Write("Invalid File Name: No proper punctuation in the file\n" +
"extension name (i.e. fileName.dat). Retry (y/n)? ");
tf = false;
break;
}
else if (fileNameSubStr.Remove(0, fileNameSubStr.LastIndexOf(".")).Length - 1 != 3)
{
//OPTIMIZATION: This block won't run 
//Check if the file extension contains exactly 3 characters
Console.Write("Invalid File Name: The extension does not strictly\n" +
"contain three letters (a-z or A-Z). Retry (y/n)? ");
tf = false;
break;
}
else if (!fileNameSubStrRegexControl.IsMatch(fileNameSubStr.Remove(0, fileNameSubStr.LastIndexOf(".") + 1)))
{
/*OPTIMIZATION: This block won't run */
//Check if the file extension contains only letters
Console.Write("The end of file extension name does contain three characters, but\n" +
"not all of them are strictly letters (a-z or A-Z). Retry (y/n)? ");
tf = false;
break;
}
foreach (var item in getInvalidPathChars)
if (fileNameSubStr.Remove(fileNameSubStr.LastIndexOf(".")).Contains(item.ToString()))
{
Console.Write("Invalid File Name: File name contains at least\n" +
"one invalid character. Retry (y/n)? ");
tf = false;
break;
}

if (tf)
{
//Console.Write("'{0}' is a valid full path file name.", directoryPathSubStr + fileNameSubStr);
//break;
}
else
{
//Console.Write("'{0}' is a not valid full path file name.", directoryPathSubStr + fileNameSubStr);
//break;
}
break;
}

directoryPathSubStr = fileNameSubStr = string.Empty;
position = 0;
return tf;
}

static void Main(string[] args)
{
/*Data Write into and Retrieve from File*/

string fullPathFileName = string.Empty;
//string filePath = string.Empty;
string directoryPath = "";
string fileName = "";
string[] customers =
{
"Bob Talbot",
"Susan Abbey", 
"Helen Murray",
"David Wright"
};
bool tf = true;

while (tf)
{
Console.ForegroundColor = ConsoleColor.DarkCyan;
Console.WriteLine("Please, enter a valid full path file name: ");
tf = ControlFullPathNameValidation(fullPathFileName = Console.ReadLine());

if (tf)
{
Console.ForegroundColor = ConsoleColor.DarkCyan;
Console.WriteLine("You entered a valid full name.");
break;
}
else
{
string yn = "";
do
{
yn = Console.ReadLine();
if (yn != "y" && yn != "n")
Console.WriteLine("Invalid character typed.\nOnly 'y' or 'no' please: ");
else if (yn == "y")
tf = true;
} while (yn != "y" && yn != "n"); 
}
}
Console.ForegroundColor = ConsoleColor.DarkCyan;

if (tf)
{
directoryPath = fullPathFileName.Remove(fullPathFileName.LastIndexOf(@"\"));
fileName = fullPathFileName.Remove(0, fullPathFileName.LastIndexOf(@"\") + 1);

//Directory Creation
if (!Directory.Exists(directoryPath))//Control directory existence
Directory.CreateDirectory(directoryPath);
else if (!File.Exists(fullPathFileName))
File.Create(fullPathFileName).Close();//Do not forget to close file

File.WriteAllLines(fullPathFileName, customers); 

directoryPath = fileName = string.Empty;

foreach (var customer in File.ReadLines(fullPathFileName))
Console.WriteLine($"Customer: {customer}");
}
else
{
Console.WriteLine("");
}

Console.ReadLine();
}
}

/*P.S.: Please, do not forget to write a few lines so as to reject any forbidden directory of file name assignment. Exclude: CON, PRN, AUX, NUL, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, LPT9 */

\$\endgroup\$
  • 3
    \$\begingroup\$ While there are inline comments, please explain the differences so that the author and other readers can learn from your thought process. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 8 '18 at 15:39
  • 2
    \$\begingroup\$ The provided code is not readable in its current form. The provided assumes false e.g c:\t.t is a valid file but based on the code a valid filename has to be at least 8 characters long. If I enter c:\t.txt the code tells me that the provided code is invalid because it doesn't take into account that the user may have the driveletter in lower caps. Hence -1 \$\endgroup\$ – Heslacher Jan 8 '18 at 17:21
  • 1
    \$\begingroup\$ While the OP asked "How could I make this be more elegant?", which does seem like it might be asking for just alternative code, "Short answers are acceptable, as long as you explain your reasoning." -see How to answer. That page also states: "Every answer must make at least one insightful observation about the code in the question." \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 8 '18 at 17:26
  • 1
    \$\begingroup\$ @GeorgeVendras: the point of this site is to study the code in the question and to 'answer' with observations and remarks that help the asker to improve their code (and skills). Posting a different implementation without any explanation isn't helpful to the asker, especially not when it doesn't work correctly (path lengths, extensions, network locations), takes more code to do less (not taking reserved filenames into account), and is generally quite sloppy (unused and irrelevant code, unhelpful comments, bad formatting, etc.). \$\endgroup\$ – Pieter Witvoet Jan 8 '18 at 19:38
  • 2
    \$\begingroup\$ In its current form, the missing indentation (among others) prevents this answer from getting any upvotes. \$\endgroup\$ – Roland Illig Jan 8 '18 at 19:54

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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