C# photo sorter

Question

I am quite new to C#, currently working through a number of pluralSight courses.

I've written a few small things here and there, but never anything that performed an entire purpose/function from start to finish, until now, with this app.

The app lists all the JPEG photos in an import folder and collects the takenDate from the exif, it then sorts it into the following structure:

D:\Photos\2014\12-2014\25-12-2014.

You can check out the full code on github
The app has been tested with around 3-4,000 photos and it churns through them in about 4-5 seconds..

A few things which I am unsure if I have done correctly are as follows:

Have I loaded my main method down with too much?

static void Main(string[] args)
        {
            log4net.Config.XmlConfigurator.Configure(); //load the logger config settings.
            log.Info("Application Started");
            string[] Files = FileHandler.ListFiles();

            foreach (var f in Files)
            {
                log.Info("---WORKING WITH THE FOLLOWING FILE---");
                log.Info(f);

                //collect all the required infomation.
                DateTime ExtractedDateTime = ExifParser.getTakenDateTime(f);
                string fileName = Path.GetFileName(f);
                string originalPath = f;
                string targetPath = FilePathBuilder.calculateTargetPath(ExtractedDateTime, fileName);

                //Do final move of file.
                FileMover.MoveToDestinationFolder(originalPath,targetPath);
            }
            ApplicationTerminator.SucessfulApplicationEnd();
        }

This is also my first implementation using log4net. Some feedback on how it is used would be great!

I am trying to keep my coding in line with things like SOLID and keeping KISS in mind with everything, I feel I have abstracted out the functionality into classes which perform only one function such as these:

public class ExifParser
{

    private static readonly ILog log = LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType);

    public static DateTime getTakenDateTime(string filePath)
    {
        ExifReader reader = new ExifReader(filePath);
        DateTime datePictureTaken;
        try
        {
            log.Info("Getting Taken Date From Photos EXIF Data");
            log.Debug("Processing EXIF Date for the following file:");
            log.Debug(filePath);

            if (reader.GetTagValue<DateTime>(ExifTags.DateTimeDigitized, out datePictureTaken))
            {
                log.Debug("EXIF Data Collected Sucessfully");
                reader.Dispose();
                return datePictureTaken;
            }
            else
            {
                reader.Dispose();
                log.Error("Failed To Collect EXIF Data, Check the following file for EXIF Data:");
                log.Error("" + filePath);
                return new DateTime(0001, 01, 01); //The FolderPath builder will recognize a date as 0001 for the year as an error and build a path to the error folder!
            }


        }
        catch (Exception e)
        {
            log.Error("An Exception Occured While Collecting EXIF Data");
            log.Error("Exception Message: " + e.Message);
            log.Error("Exception Stack: " + e.StackTrace);
            log.Error("Returning the error datetime.");
            return new DateTime(0001, 01, 01);
        }
    }

}

---

class DirectoryHandler
{
    private static readonly ILog log = LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType);

    /// <summary>
    /// Validates if directory already exists, if it does not it is created
    /// </summary>
    /// <param name="TargetPath">The Path The Files Target/Destination directory.</param>
    public static void ValidateDirectory(string TargetPath)
    {
        log.Debug("Checking if The following Directory Exists: " + Path.GetDirectoryName(TargetPath));
        if (!Directory.Exists(Path.GetDirectoryName(TargetPath)))
        {
            log.Debug("The Directory Does not Exist, attempting to create it.");
            Directory.CreateDirectory(Path.GetDirectoryName(TargetPath)); //path without the extension!
            log.Debug("Destination Directory Created!");
        }
        else
        {
            log.Debug("The directory Exists!");
        }
    }
}

Overall, I am quite happy with how the project has gone and the functionality it performs completes the task 100%! However I think some feedback on ways to improve how my code is written is one of the main things I'd like to gain from this review.

Answer 1

DirectoryHandler

The ValidateDirectory() method does not validate the directory but check if the directory exists and if it doesn't exists it will create the directory.

The checking if the directory exists can be removed, because the Directory.CreateDirectory() method internal checks if the directory exists and if it exists, it just return.

So let us rename the method to CreateDirectory().

The stripping of the path separator \ by calling Path.GetDirectoryName() isn't needed either, because it is taken care inside the Directory.CreateDirectory() method.

This would leave

public static void ValidateDirectory(string TargetPath)
{
    Directory.CreateDirectory(TargetPath);
}

which could be replaced in the calling code by System.IO.Directory.CreateDirectory(TargetPath);

ExifParser

Because ExifReader implements IDisposable it should be enclosed insied a using block. In this way it is automatically taken care of disposing the ExifReader.

The getTakenDateTime() method should return a nullable DateTime. This would provide a good way to return an "invalid" DateTime as null. Also based on the naming guidelines method names should be named using PascalCase casing.

Refactoring

public static DateTime? GetTakenDateTime(string filePath)
{
    using (ExifReader reader = new ExifReader(filePath))
    {
        try
        {
            log.Info("Getting Taken Date From Photos EXIF Data");
            log.Debug("Processing EXIF Date for the following file:");
            log.Debug(filePath);

            DateTime datePictureTaken;    
            if (reader.GetTagValue<DateTime>(ExifTags.DateTimeDigitized, out datePictureTaken))
            {
                log.Debug("EXIF Data Collected Sucessfully");
                return datePictureTaken;
            }

            log.Error("Failed To Collect EXIF Data, Check the following file for EXIF Data:");
            log.Error(filePath);
        }
        catch (Exception e)
        {
            log.Error("An Exception Occured While Collecting EXIF Data");
            log.Error("Exception Message: " + e.Message);
            log.Error("Exception Stack: " + e.StackTrace);
            log.Error("Returning the null datetime.");
        }
    }
    return null; 
}

Main

• Also the naming guidelines don't explicitly mention names of variables which are local to methods, you should consider to use camelCase casing for naming.

• variable names shouldn't be shortened so foreach (var f in Files) would look better like foreach (var file in files).

Implementing the points above will lead to

static void Main(string[] args)

{
    log4net.Config.XmlConfigurator.Configure(); //load the logger config settings.
    log.Info("Application Started");
    string[] files = FileHandler.ListFiles();

    foreach (var file in Files)
    {
        log.Info("---WORKING WITH THE FOLLOWING FILE---");
        log.Info(file);

        //collect all the required infomation.
        DateTime? extractedDateTime = ExifParser.GetTakenDateTime(file);
        string fileName = Path.GetFileName(file);
        string originalPath = file;
        string targetPath = FilePathBuilder.calculateTargetPath(extractedDateTime, fileName);

        //Do final move of file.
        FileMover.MoveToDestinationFolder(originalPath,targetPath);
    }
    ApplicationTerminator.SucessfulApplicationEnd();
}

FilePathBuilder

Because we have made changes to the ExifParser.GetTakenDateTime() method we need to adjust the FilePathBuilder class too.

So you will get an additional review for free !

• input parameters should be named using camelCase casing.

• variables should be declared as near to their usage than possible. In addition you should create variables only if they are needed or in other words if you only need one array element you don't need to create 5 elements.

  So we better should use an IList<String> on which we call ToArray() for using Path.Combine(). In this way if you e.g decide to not use the year anymore, this will be much easier than rearangeing the array elements. By the way the using of Path.Combine() is very good.

• by using early returns you can save horizontal space which improves readability.

• By extracting the composition of the error path, the getting of the outputPath and the composition of the target path, you will gain readability.

• Because you don't calculate but compose the path, we should rename the calculateTargetPath() method to ComposeTargetPath().

Usually I wouldn't extract the getting of the outputPath but because you love to log, it will reduce code duplication.

private static string GetOutputPath()
{
    log.Debug("Getting Output Path From Config file.");
    String outputPath = ConfigurationManager.AppSettings["outputPath"];
    log.Debug("Base Target Path Is: " + outputPath);
    return outputPath;
}  

private static string ComposeErrorPath(string fileName)
{
    log.Error("Date Is Incorrect Date, Building A Path To Error");
    return Path.Combine(GetOutputPath(), "Error", fileName);
}  

After extracting the logging of the year, month and day

private static void LogDateItems(string year, string month, string day)
{
    log.Debug("Take Date is:");
    log.Debug("DAY: " + day);
    log.Debug("MONTH: " + month);
    log.Debug("YEAR: " + year);
}  

and extracting the logging of the pathElements

private static void LogPathParts(IList<String> pathElements, int startIndex, int endIndex)
{
    for (int i = startIndex; i <= endIndex; i++)
    {
        log.Debug(String.Format("Level {0} Folder: {1}", i, pathElements[i]));
    }
}  

We get a simple, readable and maintainable overloaded ComposeTargetPath() method

private static string ComposeTargetPath(DateTime extractedDateTime, string fileName)
{
    log.Debug("Date Is Correct Date, continue to Build Array!");
    string day = string.Format("{0:dd}", extractedDateTime); //done like this so we get a trailing 0 for any day with only 1 digit number(1st).
    string month = string.Format("{0:MM}", extractedDateTime); //done like this so we get a trailing 0 for January.
    string year = extractedDateTime.Year.ToString();

    IList<string> pathElements = new List<string>();
    pathElements.Add(GetOutputPath());
    pathElements.Add(year); //EG: 2001
    pathElements.Add(month + "-" + year); //EG: 01-2001
    pathElements.Add(day + "-" + month + "-" + year); //EG: 22-01-2001
    pathElements.Add(fileName); //File name.

    LogDateItems(year, month, day);
    LogPathParts(pathElements, 1, 4);

    string finalPath = Path.Combine(pathElements.ToArray());
    log.Debug("Final Constructed Path: " + finalPath);

    return finalPath;
} 

which will be called by

internal static string ComposeTargetPath(DateTime? extractedDateTime, string fileName)
{
    if (extractedDateTime.HasValue)
    {
        return ComposeTargetPath(extractedDateTime.Value, fileName);
    }

    string finalPath = ComposeErrorPath(fileName);
    log.Debug("Final Constructed Path: " + finalPath);
    return finalPath;
}