Archiving files into directories

Question

This program moves files to a backup directory, archiving them by year of last modification.

From a directory like :

.
├── file1.ext
├── file2.ext
├── file3.ext
├── fileN.ext

To destination:

.
├── _Backup
|   ├── 2002
|   └──_200X
|      ├── file3.ext
|      └── fileN.ext
├── file1.ext
├── file2.ext

Configuration.xml:

<?xml version="1.0" encoding="utf-8" ?>

<data>
  <PathItem PathFrom="C:\Test\FjournaTest" 
            PathTo  ="C:\Test\FjournaTest\bak" 
            Delay="20" Enable="1" />

  <PathItem PathFrom="\\AAAAAAA\BBB\Commun\Email\XYZ\" 
            PathTo  ="\\AAAAAAA\BBB\Commun\Email\XYZ\Backup" 
            Delay="30" Enable="0" />

</data>

Delay is the number of days old the file needs to be eligible to the backup.

FileMover.Cs

class FileMove
{
    private IEnumerable<PathItem> _pathList;
    private bool _Talk = false;
    private DateTime currentDate = DateTime.Now;
    private int tryMAX = 7;

    public FileMove(string configPath)
    {
        if (!PathValid(configPath))
        {
            throw new Exception($"Invalid Configuration Path: [{configPath}].");
        }

        XDocument doc = XDocument.Load(configPath);
        var tempP = from p in doc.Descendants("PathItem")
                    where int.Parse(p.Attributes("Enable").First().Value) == 1
                    select
                     new PathItem(
                        p.Attributes("PathFrom").First().Value
                        , p.Attributes("PathTo").First().Value
                        , int.Parse(p.Attributes("Delay").First().Value)
                        , true
                    );

        //Distinct on From. 
        _pathList = tempP.GroupBy(
                            i => i.PathFrom,
                            (key, group) => group.First()
                        ).ToArray();
    }

    public FileMove(string configPath, bool talk) : this(configPath)
    {
        this._Talk = talk;
    }

    public void Run()
    {
        if (!_pathList.Any())
        {
            W("No Enabled Path.");
            return;
        }

        I("Process Start.");
        foreach (var x in _pathList)
        {
            I($"Processing:\n{x}");//TODO Try > Catch > Log.
            Process(x.PathFrom, x.PathTo, x.Delay);
        }

        I("Process End.");

    }

    private void Process(string pathFrom, string pathTo, int delay)
    {
        bool ValidPathFrom = PathValid(pathFrom)
             , ValidPathTo = PathValid(pathTo);

        if (!(ValidPathFrom && ValidPathTo))
        {
            W("Path Not Valid!");
            W($"\tPathFrom:{ValidPathFrom}\tPathTo:{ValidPathTo}");

            return;
        }

        I($"\tPath Valid  >PathFrom:{pathFrom}\t>PathTo:{pathTo}");
        string[] fileEntries = Directory.GetFiles(pathFrom);

        Parallel.ForEach(fileEntries, (fileName) =>
        {
            ProcessFile(fileName, pathTo, delay);
        });

    }


    private void ProcessFile(string fileName, string pathTo, int delay)
    {

        var fileInfo = new FileInfo(fileName);

        DateTime lastModified = fileInfo.LastWriteTime;
        int DuplicateCounter = 0;

        var yearDirectory = lastModified.Year.ToString();
        string shortName = Path.GetFileNameWithoutExtension(fileInfo.Name);

        pathTo += $"\\{yearDirectory}";
        string savePath = $"{pathTo}\\{fileInfo.Name}";

        if (delay == 0 || (currentDate - lastModified).Days >= delay)
        {
            //Year Dir Exist ?
            if (!PathValid(pathTo))
            {
                Directory.CreateDirectory(pathTo);
            }

            // Make sure that the target does not exist.
            while (File.Exists(savePath) && DuplicateCounter < 99)
            {
                DuplicateCounter++;
                savePath = $"{pathTo}\\{shortName}({DuplicateCounter}){fileInfo.Extension}";
                if (DuplicateCounter == 99) {
                     W($"\t\t[{shortName}] Have to many duplicate.");
                }
            }

            // MoveIt.
            int tryCount = 0;
            while (tryCount < tryMAX + 1)
            {
                try
                {
                    fileInfo.MoveTo(savePath);
                }
                catch (Exception e)
                {
                    if (tryCount == tryMAX)
                    {//thinks 7 time before it speaks
                        throw new Exception($"-File move : {savePath} #Failed", e);
                    }
                    tryCount++;
                    continue;
                }
                break;
            }
            I("{0} was moved to {1}.", fileName, savePath);
        }
    }

    private bool PathValid(string path)
    {
        if (File.Exists(path))
        {
            return true;
        }
        else if (Directory.Exists(path))
        {
            return true;
        }
        else
        {
            W("{0} is not a valid directory or file.", path);
            return false;
        }
    }
    private bool PathValid(string path, out bool validPathFrom)
    {
        validPathFrom = PathValid(path);
        return validPathFrom;
    }

    public void W(string format, params object[] args)  //Warning always printed
    {
    #if DEBUG
        System.Diagnostics.Debug.WriteLine(format, args);
    #endif
        Console.WriteLine(format, args);
    }

    public void I(string format, params object[] args)  //InfoCut off if not talkative.
    {
        if (_Talk)
            W(format, args);
    }
}

Answer 1

Several additions to the answer of @t3chb0t.

This

private int tryMAX = 7;

should be a constant:

private const int TryMax = 7;

---

You should never throw an instance of the Exception class. Instead use apropriate built-in class or design your own exception. So instead of

if (!PathValid(configPath))
{
    throw new Exception($"Invalid Configuration Path: [{configPath}].");
}

you should use

if (!PathValid(configPath))
{
    throw new ArgumentException($"Invalid Configuration Path: [{configPath}].", nameof(configPath));
}

And here

if (tryCount == tryMAX)
{//thinks 7 time before it speaks
    throw new Exception($"-File move : {savePath} #Failed", e);
}

you can use InvalidOperationException, for example:

throw new InvalidOperationException($"-File move : {savePath} #Failed", e);

---

Instead of

p.Attributes("PathFrom").First().Value

you can use Attribute method:

p.Attribute("PathFrom").Value

Also XAttribute have several explicit conversion operators so I recommend to use them. This

p.Attribute("PathFrom").Value

can be replaced with

(string)p.Attribute("PathFrom")

Especially it is useful when the desired type is not string. So instead of

int.Parse(p.Attributes("Delay").First().Value)

you can write

(int)p.Attribute("Delay")

So all this code

var tempP = from p in doc.Descendants("PathItem")
            where int.Parse(p.Attributes("Enable").First().Value) == 1
            select
             new PathItem(
                p.Attributes("PathFrom").First().Value
                , p.Attributes("PathTo").First().Value
                , int.Parse(p.Attributes("Delay").First().Value)
                , true
            );

will be simplified to

var tempP = from p in doc.Descendants("PathItem")
            where (int)p.Attribute("Enable") == 1
            select new PathItem((string)p.Attribute("PathFrom"),
                                (string)p.Attribute("PathTo"),
                                (int)p.Attribute("Delay"),
                                true);

Answer 2

public void W(string format, params object[] args)  //Warning always printed
{
    ..
}

public void I(string format, params object[] args)  //InfoCut off if not talkative.
{
    ..
}

W and I? Yuck! What happened to the rest of these names?

---

var fileInfo = new FileInfo(fileName);

DateTime lastModified = fileInfo.LastWriteTime;

This is very inconsistent. Once you use a full type name and another time just the var and this interchangeably. Not sure which one is better? I suggest using var everywhere.

private IEnumerable<PathItem> _pathList;
private bool _Talk = false;
private DateTime currentDate = DateTime.Now;
private int tryMAX = 7;

The same goes for the fields. Some of them are prefixed with an underscore, the other ones not.

---

pathTo += $"\\{yearDirectory}";
string savePath = $"{pathTo}\\{fileInfo.Name}";

We don't concatenate paths like this. You should use the Path.Combine method for this job.

---

//Year Dir Exist ?
if (!PathValid(pathTo))
{
  Directory.CreateDirectory(pathTo);
}

This comment would be unnecessary if you named the variables properly. For example instead of pathTo you could say yearDirectoryName. The PathValid method is also not helping. What does it mean that a path is valid? Is it valid because it doesn't contain any invalid characters or is is valid because it exits? You should decide and use the proper method. If you check if something exists then call it Exists if you validate it then call it ValidatePathDoesNotContainInvalidCharacters but with all the Ws and Is such a long name might be a problem I guess ;-)

---

// Make sure that the target does not exist.
while (File.Exists(savePath) && DuplicateCounter < 99)
{
  DuplicateCounter++;
  savePath = $"{pathTo}\\{shortName}({DuplicateCounter}){fileInfo.Extension}";
  if (DuplicateCounter == 99)
  {
      W($"\t\t[{shortName}] Have to many duplicate.");
  }
}

How does this make sure the target does not exist? It counts existing paths so put it a new method and call it GetLastDuplicateNumber. You should also craete a constant for the magic 99.

---

private bool PathValid(string path)
{
  if (File.Exists(path))
  {
      return true;
  }
  else if (Directory.Exists(path))
  {
      return true;
  }
  else
  {
      W("{0} is not a valid directory or file.", path);
      return false;
  }
}

This method could be called PathExists becasue this is what it does. It doesn't validate anything.

---

Overall I find this code is very messy and contains a lot of misleading names and comments. You should work on smaller methods with precise descriptive names so that you don't have to write comments trying to explain what you code does. Comments are to explain why you are doing something, not what or how.

Answer 3

Other answers already reviewed your code, I'd add a small change to your architecture. You already have a PathItem class (which you do not show here) to hold the configuration of each path to handle. Write it to be XAML-friendly and parsing will be pretty easy. This code:

XDocument doc = XDocument.Load(configPath);
 var tempP = from p in doc.Descendants("PathItem")
        where int.Parse(p.Attributes("Enable").First().Value) == 1
        select
         new PathItem(
            p.Attributes("PathFrom").First().Value
            , p.Attributes("PathTo").First().Value
            , int.Parse(p.Attributes("Delay").First().Value)
            , true
        );

Will then be:

var tempP = (PathItem[])XamlServices.Load(configPath);

Much easier and you get all the power of the XAML parser.

---

Now in your Run() method you have this code:

foreach (var x in _pathList)
     Process(x.PathFrom, x.PathTo, x.Delay);

Why do you need to expand PathItem properties as parameters? If tomorrow you will add a new property (for example a SearchPattern of a filter based on file attributes or both) then you will need to also add more and more parameters to Process(). Simply:

 Array.ForEach(_pathList, Process);

Where Process() has this prototype:

private void Process(PathItem pathItem);

---

You're using Parallel.ForEach() inside Process() to move files in parallel. Even if, at first, it might look like a good idea you should also limit the number of parallel operations. According to How many threads Parallel.ForEach will create? Default MaxDegreeOfParallelism? you may have a (variable) high number of threads but move operation is limited by two factors:

• Source and target disk throughput if source and target directory are on different drives.
• Kernel locks for cached file system entries.

In both cases a, let's say, 20/30 parallel copies are an unreasonable value which may end slowing down your copy operation. You should limit the number of parallel operations of Parallel.ForEach() to a much lower value (see also Find and rename all png files). I'd say to use the number of CPU cores as limit but in a comment Peter Cordes suggested to start experimenting with 8 then probably the double of the average number of cores of today's CPUs (and if he says so then I'd almost blindly pick that value as the best one). Things may change in future about cores but you will always have to face the I/O bandwidth (and, if you're not using SSDs, it's synchronous serial nature) limitations.