4
\$\begingroup\$

Overview
I wanted to create a small console app that would point at a given directory, evaluate the last accessed date of each file within that directory, and if the last access date was below a certain threshold, archive that file. The archive folder structure should match that of the source directory. I also decided to output the source folder structure to a log file, with an "ARCHIVED" flag against all those files that were archived.

Request
If you wouldn't mind reviewing my code for inefficiencies, mistakes, duplications, and anything else you think could be improved upon.

Note
I have included a decompress function which is not utilised in the script, but I needed for testing purposes.

Code

using System;
using System.Collections.Generic;
using System.IO;
using System.IO.Compression;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Archiving
{
    class Program
    {
        static void Main(string[] args)
        {
            string sourcePath = @"e:\source\";
            string destPath = @"e:\archive";
            string logPath = @"e:\log files\";
            string currentDate = DateTime.Now.ToShortDateString().Replace("/", "");

            using (StreamWriter streamWriter = File.CreateText(logPath + currentDate + ".txt"))
            {
                try
                {
                    string[] directories = Directory.GetDirectories(sourcePath, "*", SearchOption.AllDirectories);
                    foreach (string directory in directories)
                    {
                        streamWriter.WriteLine(directory);
                        string[] dirFiles = Directory.GetFiles(directory, "*.*", SearchOption.TopDirectoryOnly);
                        foreach (string file in dirFiles)
                        {
                            FileInfo fileInfo = new FileInfo(file);
                            if (FileExpiry(fileInfo))
                            {
                                string trimmedDirPath = fileInfo.DirectoryName.Substring(sourcePath.Length);
                                string archiveDirPath = Path.Combine(destPath, trimmedDirPath);
                                // if directory doesn't exist in archive, create
                                if (!Directory.Exists(archiveDirPath))
                                    Directory.CreateDirectory(archiveDirPath);
                                // compress file
                                Compress(fileInfo);
                                // if file exists, append datetime to file name
                                if (File.Exists(Path.Combine(archiveDirPath, fileInfo.Name + ".gz")))
                                    File.Move(fileInfo.FullName + ".gz", Path.Combine(archiveDirPath, currentDate + fileInfo.Name + ".gz"));
                                else
                                    File.Move(fileInfo.FullName + ".gz", Path.Combine(archiveDirPath, fileInfo.Name + ".gz"));
                                // remove original file
                                File.Delete(fileInfo.FullName);
                                // append 'ARCHIVED' to files that have expired
                                streamWriter.WriteLine("     " + Path.GetFileName(file) + "  - Archived");
                            }
                            else
                            {
                                streamWriter.WriteLine("     " + Path.GetFileName(file));
                            }
                        }
                    }
                }
                catch (DirectoryNotFoundException ex)
                {
                    streamWriter.WriteLine(ex.Message);
                }
                catch (IOException ex)
                {
                    streamWriter.WriteLine(ex.Message);
                }
                catch (Exception ex)
                {
                    streamWriter.WriteLine(ex.Message);
                }
            }
        }
        public static bool FileExpiry(FileInfo fileInfo)
        {
            if (fileInfo.LastAccessTime < DateTime.Now.Date.AddDays(-5))
                return true;
            else
                return false;
        }
        public static void Compress(FileInfo fileToCompress)
        {
            using (FileStream fileStream = fileToCompress.OpenRead())
            {
                if ((File.GetAttributes(fileToCompress.FullName) &
                    FileAttributes.Hidden) != FileAttributes.Hidden & fileToCompress.Extension != ".gz")
                {
                    using (FileStream compressedFileStream = File.Create(fileToCompress.FullName + ".gz"))
                    {
                        using (GZipStream compressionStream = new GZipStream(compressedFileStream,
                            CompressionMode.Compress))
                        {
                            fileStream.CopyTo(compressionStream);
                        }
                    }
                }
            }
        }
        public static void Decompress(FileInfo fileToDecompress)
        {
            using (FileStream fileStream = fileToDecompress.OpenRead())
            {
                string currentFileName = fileToDecompress.FullName;
                string newFileName = currentFileName.Remove(currentFileName.Length - fileToDecompress.Extension.Length);

                using (FileStream decompressedFileStream = File.Create(newFileName))
                {
                    using (GZipStream decompressionStream = new GZipStream(fileStream,
                        CompressionMode.Decompress))
                    {
                        decompressionStream.CopyTo(decompressedFileStream);
                    }
                }
            }
        }
    }
}
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$
  1. It's better to use App.config file to store such app settings as source path and destination path. It might have sense to provide these parameters via application args too. Hardcoding these settings is not the best option because you might run the application on system without e drive.
  2. I suggest you use some logging library for logging in your application. I personally prefer NLog. So logPath setting and streamWriter will go away from your code.
  3. Your code currently have too many levels of indentation. That result in too big method with big context which should be kept in mind.
  4. There is no need to catch DirectoryNotFoundException and IOException separately, because you are handling them in exactly same way as global Exception. Even more - you can use global UnhandledException handler to catch and log all unhandled exceptions.
  5. You can use EnumerateDirectories and EnumerateFiles methods to avoid grabbing all entries into array.
  6. Of course you can move all file-arhiavation logic to some separate class (e.g. Arhiver) and call that class from simple and clean Main method.
  7. Try not to use magic numbers in your code. E.g. it's not clear that expiration time is 5 days. It's better to create some constant variable with expressive name for that number or even provide this value from args/config.
  8. Writing if <condition> return true else return false is totally same as return <condition>.
  9. If you want to get all files from given directory you don't need to specify search pattern and search option.

After these improvements your console application code will be simple and clear - get application settings, set error handler and start archivation of expired files:

class Program
{
    private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

    static void Main(string[] args)
    {
        var sourcePath = ConfigurationManager.AppSettings["sourcePath"];
        var destinationPath = ConfigurationManager.AppSettings["destinationPath"];
        AppDomain.CurrentDomain.UnhandledException +=
           (o, e) => Logger.Error("An error occured", e);

        var archiver = new Archiver();
        archiver.ArchiveExpiredFiles(sourcePath, destinationPath);
    }
}

Archiver is also pretty straight-forward - it enumerates source directories and archives each expired file:

public class Archiver
{
    private TimeSpan expirationInterval = TimeSpan.FromDays(5);
    private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

    public void ArchiveExpiredFiles(string sourcePath, string destPath)
    {
        // you can check arguments here
        var expirationDate = DateTime.Now - expirationInterval;
        var sourceDirectory = new DirectoryInfo(sourcePath);
        var destDirectory = new DirectoryInfo(destPath);

        foreach (var sourceSubDirectory in sourceDirectory.EnumerateDirectories("*", SearchOption.AllDirectories))
        {
            var relativePath = sourceSubDirectory.GetPathRelativeTo(sourceDirectory);
            var destSubDirectory = new DirectoryInfo(Path.Combine(destPath, relativePath));

            ArchiveExpiredFiles(sourceSubDirectory, destSubDirectory, expirationDate);
        }
    }

    private void ArchiveExpiredFiles(DirectoryInfo sourceDirectory, DirectoryInfo destDirectory, DateTime expirationDate)
    {
        Logger.Info($"Processing directory {sourceDirectory.FullName}");

        foreach (var file in sourceDirectory.EnumerateFiles())
        {
            if (!file.IsExpired(expirationDate))
            {
                Logger.Info($"\t{file.FullName}");
                continue;
            }

            MoveFileToArchive(file, destDirectory);
            Logger.Info($"\t{file.FullName} - Archived");
        }
    }

    private void MoveFileToArchive(FileInfo file, DirectoryInfo destDirectory)
    {
        if (!destDirectory.Exists)
            destDirectory.Create();

        var compressedFile = file.IsCompressed() ? file : file.Compress();

        var destFilePath = Path.Combine(destDirectory.FullName, compressedFile.Name);
        if (File.Exists(destFilePath))
            destFilePath = Path.Combine(destDirectory.FullName, DateTime.Today.ToString("yyyyMMdd"), compressedFile.Name);

        compressedFile.MoveTo(destFilePath);
        file.Delete();
    }
}

Last missing part is extensions which I used in archiver - compressing files, checking wheter file was compressed or if its expired will look much cleaner

public static class FileSystemInfoExtensions
{
    public static bool IsExpired(this FileInfo file, DateTime expirationDate)
    {
        return file.LastAccessTime < expirationDate;
    }

    public static FileInfo Compress(this FileInfo file)
    {
        // file-compression
        throw new NotImplementedException();
    }

    public static bool IsCompressed(this FileInfo file)
    {
        return file.Extension == ".gz";
    }

    public static bool IsHidden(this FileInfo file)
    {
        return (file.Attributes & FileAttributes.Hidden) == FileAttributes.Hidden;
    }

    public static string GetPathRelativeTo(this DirectoryInfo subDirectory,
        DirectoryInfo directory)
    {
        if (!subDirectory.FullName.StartsWith(directory.FullName))
            throw new ArgumentException(
               $"{directory.FullName} is not parent of {subDirectory.FullName}");

        return subDirectory.FullName.Substring(directory.FullName.Length + 1);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Thank you, I took your advice and reduced the code somewhat. The EnumerateDirectories/EnumerateFiles methods I can't say I was aware of. \$\endgroup\$ Commented Dec 5, 2016 at 16:02
  • \$\begingroup\$ @GeorgeGrainger you can consider accpeting this answer :-] \$\endgroup\$
    – t3chb0t
    Commented Dec 28, 2016 at 12:31
1
\$\begingroup\$

You can improve the Compress method by changing the order of operations. It is requried that the file doesn't have the .gz extension so you should check this first before opening any streams. This will allow you to remove some indentation and put the usings next to eeach other and make your code more readable. By using the var you can make it still shorter.

public static void Compress(FileInfo fileToCompress)
{
    var canCompress = (File.GetAttributes(fileToCompress.FullName) & FileAttributes.Hidden) != FileAttributes.Hidden & fileToCompress.Extension != ".gz";
    if (!canCompress)
    {
        return;
    }

    using (var fileStream = fileToCompress.OpenRead())
    using (var compressedFileStream = File.CreatefileToCompress.FullName + ".gz")
    using (var compressionStream = new GZipStream(compressedFileStream, CompressionMode.Compress))
    {
        fileStream.CopyTo(compressionStream);
    }
}

The same applies to the Decompress. Move the preparation lines outside and put the usings together:

public static void Decompress(FileInfo fileToDecompress)
{
    string currentFileName = fileToDecompress.FullName;
    string newFileName = currentFileName.Remove(currentFileName.Length - fileToDecompress.Extension.Length);

    using (var fileStream = fileToDecompress.OpenRead())
    using (var decompressedFileStream = File.Create(newFileName))
    using (var decompressionStream = new GZipStream(fileStream, CompressionMode.Decompress))
    {
        decompressionStream.CopyTo(decompressedFileStream);
    }
}

I advise against the app.config. Use JSON for the settings and you won't have to parse anything. You'll also be able to use complex types rather then only the integral ones.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you, I hadn't realised you could bunch using statements like that! Looks much cleaner \$\endgroup\$ Commented Dec 5, 2016 at 15:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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