Refactoring help with Ifs

Question

I've been working on this code for awhile and for some reason all the if's are driving me crazy along with a bunch of repeated code. Is there a nicer more cleaner way to do this?

public Program(string[] args)
    {
        try
        {
            WriteToLogFile("Starting ImportTask");
            if (args.Length == 0)
            {
                Import(DateTime.Now,DateTime.Now);
                MarkRecordsAsDeleted();
            }
            else if (args.Length == 1)
            {
                DateTime dateToImport;
                bool isValidDate = DateTime.TryParse(args[0], out dateToImport);
                if (isValidDate)
                {
                    Import(dateToImport,dateToImport);
                    MarkRecordsAsDeleted();
                }
                else
                    WriteToLogFile(String.Format("The Import date specified was invalid. - {0}", args[0]));
            }
            else if (args.Length == 2)
            {
                DateTime importStartDate;
                bool isValidStartDate = DateTime.TryParse(args[0], out importStartDate);
                DateTime importEndDate;
                bool isValidEndDate = DateTime.TryParse(args[0], out importEndDate);
                if (isValidStartDate && isValidEndDate)
                {
                    if (importStartDate > importEndDate)
                    {
                        WriteToLogFile(String.Format("Invalid date range provided. Start date = {0} End date {1}",importStartDate,importEndDate));
                        return;
                    }
                    Import(importStartDate, importEndDate);
                    MarkRecordsAsDeleted();
                }
                else
                    WriteToLogFile(String.Format("The Import date specified was invalid. - {0}", args[0]));
            }
            else
            {
                WriteToLogFile("Invalid Command Line Parameters Specified");
            }

        }
        catch (Exception ex)
        {
            WriteToLogFile("Error in Import Process = " + ex.StackTrace);
        }
    }

Answer 1

Take a look at the strategy pattern. It would take some major re-factoring and quite a bit of new classes but it should solve the problem you are looking to solve.

Disregarding the fancy names for a moment, the idea is actually quite simple.

You have a abstract or interface class that defines a single method to call. You then have several derived classes that you move the contents of your if in to.

As a quick example lets do the following:

 interface Ifoo
 {
     void myAction();
 }

 class MyCustomActionBar : Ifoo
  {
      public void myAction()
      {
          //.... details ... - Contents of single if statement.
      }
  }

 class MyCustomActionTar : Ifoo
  {
      public void myAction()
      {
          //.... details ... - Contents of next but single if statement.
      }
  }

Once you have all the classes setup, you'd then change around your if to be something like the following.

public Program(string[] args)
{
   Ifoo _myFoo;
    try
    {
        WriteToLogFile("Starting ImportTask");
        if (args.Length == 0)
        {
            _myFoo = new MyCustomActionBar();
        }
        else if (args.Length == 1)
        {
            _myFoo = new MyCustomActionTar ();
        }
        //... etc....
        }
        else
        {
              _myFoo = new MyErrorAction(); //Definition not illustrated above
        }
        _myFoo.myAction();   //Getting your actual return values and stuff would be design details that I'm leaving out.

    }
    catch (Exception ex)
    {
         //...
    }
}

Please excuse errors or typos in the example, I did it very quickly. It is just to illustrate the general idea to simply the presentation of the Wikipedia article to give a quick snapshot of the idea.. The details would be more involved.

Answer 2

Here's a first pass:

public Program(string[] args)
{
    try
    {
        WriteToLogFile("Starting ImportTask");
        DateTime startTime;
        DateTime endTime;
        GetDateRange(args, out startTime, out endTime);

        Import(startTime, endTime);
        MarkRecordsAsDeleted();
    }
    catch (Exception ex)
    {
        WriteToLogFile("Error in Import Process = " + ex);
        throw;
    }
}

    private static void GetDateRange(string[] args, out DateTime startTime, out DateTime endTime)
    {
        switch (args.Length)
        {
            case 0:
                startTime = DateTime.Now;
                endTime = DateTime.Now;
                break;
            case 1:
                {
                    DateTime dateToImport;
                    var isValidDate = DateTime.TryParse(args[0], out dateToImport);
                    if (!isValidDate)
                    {
                        throw new Exception(
                            String.Format(
                                "The Import date specified was invalid. - {0}",
                                args[0]));
                    }

                    startTime = dateToImport;
                    endTime = dateToImport;
                }
                break;
            case 2:
                {
                    DateTime importStartDate;
                    bool isValidStartDate = DateTime.TryParse(args[0], out importStartDate);
                    DateTime importEndDate;
                    bool isValidEndDate = DateTime.TryParse(args[1], out importEndDate);
                    if (!isValidStartDate || !isValidEndDate)
                    {
                        throw new Exception(
                            String.Format(
                                "The Import dates specified was invalid. - {0}, {1}",
                                args[0], args[1]));
                    }

                    if (importStartDate > importEndDate)
                    {
                        throw new Exception(
                            String.Format(
                                "Invalid date range provided. Start date = {0} End date {1}",
                                importStartDate, importEndDate));
                    }

                    startTime = importStartDate;
                    endTime = importEndDate;
                }
                break;
            default:
                throw new Exception("Invalid Command Line Parameters Specified");
        }
    }

Answer 3

Here's my two cents. Note that DateTime parse failures default back to DateTime.Now which is quite possibly NOT what you want. If so, just handle it how you want. From your source code, you log it and just complete without throwing an error.. Depending on how you handle problems, you would probably want to do the same thing when (startDate > endDate).

Following is untested code...

public Program(string[] args)
{
    WriteToLogFile("Starting ImportTask");

    DateTime startDate = DateTime.Now;
    DateTime endDate = DateTime.Now;

    if (args.Length > 0)                    
  if (!DateTime.TryParse(args[0], out startDate)
   WriteToLogFile(string.format("Warning: StartDate argument, {0}, is invalid; value has reverted to current Date/Time.", args[0]) );

    if (args.Length > 1)
  if (!DateTime.TryParse(args[1], out endDate)
   WriteToLogFile(string.format("Warning: EndDate argument, {0}, is invalid; value has reverted to current Date/Time.", args[]) );

    if (startDate > endDate)
    {       
  WriteToLogFile(String.Format("Invalid date range provided. Start date = {0} End date {1}",importStartDate,importEndDate));
        return;
    }

    Import(startDate, endDate);
    MarkRecordsAsDeleted();
}

Answer 4

What you've got here is a two-argument function that is expecting strings that can parse to dates. If the second argument is missing, you use the first argument; if the first is missing, you use DateTime.Now.

So, write a two-argument function that takes two dates. Call it. Something along the lines of:

switch(args.length) {
    case 0: myFunc(DateTime.Now, DateTime.Now); break;
    case 1: myFunc(toDate(args[0]), toDate(args[0])); break;
    case 2: myFunc(toDate(args[0]), toDate(args[1])); break;
}

By the way, it appears that you are not referring to args[1] in your function; that's probably a bug.