Refactoring advice/ideas needed

Question

Hi all, I was looking for ideas on improving the following code

static void Main(string[] args)
{
    bool validInput1 = false;
    string input1 = string.Empty;
    bool validInput2 = false;
    string input2 = string.Empty;

    bool validFilePath = false;
    string filePath = string.Empty;


    try
    {
        Console.WriteLine("Import process started.");

        while (!validFilePath)
        {
            Console.Write("Please enter the full path to the file you would like to import: ");
            filePath = Console.ReadLine().Replace("\"","");
            if (File.Exists(filePath))
                validFilePath = true;
        }

        while (!validInput1)
        {
            Console.Write("Enter a valid eventID the import file: ");
            input1 = Console.ReadLine();
            if (ValidEventID(input1.Trim().Length))
                validInput1 = true;
        }

        while (!validInput2)
        {
            Console.Write("Enter a valid import type code: ");
            input2 = Console.ReadLine();
            if (input2.Trim().ToUpper() == "EX" || input2.Trim().ToUpper() == "EL")
                validInput2 = true;
        }


        var records = Utilities.ParseCSV(filePath);
        var import = new Import
        {
            EventId = input1,
            ImportType = input2
        };


        import.ImportEventDates(records);

        Console.WriteLine("Import process completed.");
        Console.ReadLine();
    }
    catch (Exception ex)
    {
        Console.WriteLine("Error encountered");
        Console.WriteLine(ex.ToString());
        Console.ReadLine();
    }
}

Thanks in advance for any help

Answer 1

Why? Are you getting graded? :) It's a perfectly readable little utility, move on to the next task? :P (Like making it a GUI application so they can pick the file instead of type it?)

Answer 2

You could try taking the while loops out and putting them each in their own functions which return the valid filepath and inputs and throw exceptions if they're caught. For example:

public string FindValidFilepath()
{
    string filePath = string.Empty
    try{
        while (!validFilePath)
        {
            Console.Write("Please enter the full path to the file you would like to import: ");
            filePath = Console.ReadLine().Replace("\"","");
            if (File.Exists(filePath))
                validFilePath = true;
        }
    } catch (Exception ex) {
        throw;
    }
    return filePath
}

and call it from the Main function. Other things you can do which will reduce errors if you have to add code is to put curly braces, {, around the code within your If statements. While not having them is syntactically legal, it will keep an error from popping up later. I have been bitten by a mistake like that in the past.

EDIT: The rethrowing of this exception is so that the original Try-Catch block can stay in place. Also, I've learned that "throw ex" loses the stack trace but simply "throw" keeps it. I've corrected this issue.

EDIT 2: One more thing I thought of, try catching specific exceptions and outputting a specific error explaining what went wrong to the user so they can better understand the problem. Most users don't understand a stack trace unless outputting the stack trace is a requirement.

Also, please excuse any small syntax errors, I'm more familiar with Java than C#.

Answer 3

I'd write a simple method to retrieve and validate user input :

public static string PromptUntilValid(string promptText, Func<string, bool> validationPredicate)
{
    string input;
    do
    {
        Console.Write(promptText);
        input = Console.ReadLine();
    } while (!validationPredicate(input))
    return input;
}

This would allow your code to be refactored as follows :

    ...

    filePath = PromptUntilValid(
        "Please enter the full path to the file you would like to import: ",
        s => File.Exists(s));

    input1 = PromptUntilValid(
        "Enter a valid eventID the import file: ",
        s => ValidEventID(s.Trim().Length));

    input2 = PromptUntilValid(
        "Enter a valid import type code: ",
        s => s.Trim().ToUpper() == "EX" || s.Trim().ToUpper() == "EL");

    ...