9
\$\begingroup\$

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); } } 
\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

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); 
\$\endgroup\$
2
  • \$\begingroup\$ ` throw new Exception`.. 400+ Matches .. In 3 weeks .. I guess I would have to name things. \$\endgroup\$ Commented Aug 24, 2017 at 7:26
  • \$\begingroup\$ @DragandDrop Good luck with it :) \$\endgroup\$ Commented Aug 24, 2017 at 8:03
4
\$\begingroup\$
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.

\$\endgroup\$
3
  • \$\begingroup\$ Damn it, you beat me to it. "Discard" it is.. :) \$\endgroup\$ Commented Aug 23, 2017 at 15:31
  • \$\begingroup\$ @Abbas I'm sure you've found something worth mentioning too ;-) \$\endgroup\$ Commented Aug 23, 2017 at 15:32
  • \$\begingroup\$ "very inconsistent" How do you know my full name? It's because of this that I'm posting in CR. To apply the consistency in my every day work. So thanks for your time. \$\endgroup\$ Commented Aug 24, 2017 at 7:06
3
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ I can't wait to the max Parallel copy on our new SAN EMC! \$\endgroup\$ Commented Aug 24, 2017 at 11:50
  • \$\begingroup\$ Aahahhaahahha hit the limit! \$\endgroup\$ Commented Aug 24, 2017 at 12:30
  • \$\begingroup\$ The server admin hit faster than the server limit \$\endgroup\$ Commented Aug 24, 2017 at 12:37
  • \$\begingroup\$ Then he is Chuck Norris... \$\endgroup\$ Commented Aug 24, 2017 at 12:44

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.