PDF conversion and time stamp tool

This is the latest edition of my PDF tool, previously mentioned here, here, here, and inspired by my first attempt at a MVP here.

Major differences from previous editions:

• It uses the MVP pattern (as opposed to the god-tier UI that did everything).

• The background/separate thread is gone. I will consider adding it back later, but it was giving me pains with the new pattern.

• I "fixed" every last using directive.

• I removed every last documentation comment.

• The processor is now a static class.

"Problems":

• The UI/View is still not entirely naive. It knows what scripts and fields are, and assigns them depending on which button the user presses. I don't know if there is a better alternative or not.

• The presenter seems to do a little too much direct modification to the UI. Again, not sure what can/should be done about that.

• The background thread is gone. This means that very large workloads have the potential to crash the application, due to the Winforms timeout. This can always be avoided by not running several thousand files at a time, but I feel I should do something to "fix" this.

• The "model" part of the MVP is largely nonexistent. There is simply no data that needs to be handled save a list of strings. The back end only handles actions, which largely do not affect the view.

PdfConversionAndTimeStampTool.cs

```
namespace PdfConversionAndTimeStampTool
{
using System.IO;
using Application = System.Windows.Forms.Application;

internal static class PdfConversionAndTimeStampTool
{
[System.STAThread]
private static void Main()
{
Application.EnableVisualStyles();
Application.SetCompatibleTextRenderingDefault(false);

var view = new PdfConversionAndTimeStampToolView();
var presenter = new PdfConversionAndTimeStampToolPresenter(view);
Directory.CreateDirectory(PdfProcessor.OutputPath);
Directory.CreateDirectory(PdfProcessor.Processing

You have a bug in this "setter".

public List FileNames
    {
        get
        {
            return openFileDialog.FileNames.ToList();
        }

        set
        {
            foreach (string fileName in value)
            {
                fileView.Items.Add(fileName, isChecked: true);
            }
        }
    }

You're not actually setting the list. You're appending to it. Setting it twice will result in simply adding all of the new values to the end.

PrepareFile does nothing useful. It just delegates off to another static method that does exactly the same thing.

internal static string CopyFileToProcessing(string filename)
    {
        var processingPath = GetProcessingPath(filename);
        File.Copy(filename, processingPath);
        return processingPath;
    }

    internal static string PrepareFile(string fileName)
    {
        return CopyFileToProcessing(fileName);
    }

    internal static List PrepareFiles(List fileNames)

This FileProcessor class is also static, I recommend against it. Later, you're going to want to mock it out so you don't hit an actual file system during testing. In order to do that, you'll want to declare an interface and interfaces can't have static methods.

I really dislike this method.

internal static List PrepareFiles(List fileNames)
    {
        for (int i = 0; i < fileNames.Count; i++)
        {
            fileNames[i] = CopyFileToProcessing(fileNames[i]);
        }
        return fileNames;
    }

First and foremost, you're modifying the List that you passed in. That's a terribly surprising side effect. You could be creating yourself a nasty bug due to unexpected global state. Minimally, it will be harder to maintain due to that global state. Much better to build a new list. And if you're going to build a new list, you might as well use a foreach.

internal static List PrepareFiles(List fileNames)
    {
        List result;
        foreach (var fileName in fileNames)
        {
            result.Add( CopyFileToProcessing(fileName));
        }
        return result;
    }

Which could easily be rewritten as some Linq.

internal static List PrepareFiles(List fileNames)
    {
        return fileNames.Select(CopyFileToProcessing).ToList();
    }

Although, I would probably return an IEnumerable instead and omit the ToList() call so that it could be evaluated lazily.

StackExchange Code Review Q#131366, answer score: 4