HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

C# photo sorter

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sorterphotostackoverflow

Problem

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 = Lo

Solution

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(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 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 Da

Code Snippets

public static void ValidateDirectory(string TargetPath)
{
    Directory.CreateDirectory(TargetPath);
}
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; 
}
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();
}
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);
}
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);
}

Context

StackExchange Code Review Q#77453, answer score: 3

Revisions (0)

No revisions yet.