2
\$\begingroup\$

My program is iterating over list of files and processing each of them using some library. It's possible that file might be corrupted in the process so I want to backup it to prevent potential corruption.

I implemented it this way:

private static void ComprocessFiles(List<string> files)
{
    FileInfo file = null;                    
    foreach (string filepath in files)
    {
        bool backedUp = false;
        bool keepBackup = false;
        try
        {
            file = new FileInfo(filepath);
            if (!file.Exists) continue;
            if (file.IsReadOnly) continue;

            BackupHelper.Backup(file);
            backedUp = true;
            ProcessFile(file);

            BackupHelper.RestoreTimestamps(file);
            file.Refresh();
        }
        catch (Exception ex)
        {
            try
            {
                Logger.LogSkippedFile(ex.GetType().Name, ex.Message, filepath);
                if (backedUp) BackupHelper.RestoreFile(file);
            }
            catch (Exception ex)
            {
                keepBackup = true;
            }
            // Listed exceptions are not critical.
            if (!(ex is IOException || ex is UnauthorizedAccessException || ex is System.Security.SecurityException))
            {
                throw;
            }
        }
        finally
        {
            if (!IsCorrupted(file)) keepBackup = true;
            if (!keepBackup) BackupHelper.DeleteBackup(file);
        }
    }
}

BackupHelper class:

static class BackupHelper
{
    static private DateTime[] timestamps = new DateTime[3];

    static internal void Backup(FileInfo originalFile)
    {
        timestamps[0] = originalFile.CreationTime;
        timestamps[1] = originalFile.LastWriteTime;
        timestamps[2] = originalFile.LastAccessTime;

        File.Copy(originalFile.FullName, originalFile.FullName + "_backup", true);
    }

    static internal void Backup(string originalFile)
    {
        Backup(new FileInfo(originalFile));
    }

    static internal void RestoreFile(FileInfo originalFile)
    {
        File.Copy(originalFile.FullName + "_backup", originalFile.FullName, true);
    }

    static internal void RestoreFile(string originalFile)
    {
        RestoreFile(new FileInfo(originalFile));
    }

    static internal void RestoreTimestamps(FileInfo originalFile)
    {
        originalFile.CreationTime   = timestamps[0];
        originalFile.LastWriteTime  = timestamps[1];
        originalFile.LastAccessTime = timestamps[2];
    }

    static internal void RestoreTimestamps(string originalFile)
    {
        RestoreTimestamps(new FileInfo(originalFile));
    }

    static internal void DeleteBackup(FileInfo originalFile)
    {
        timestamps = new DateTime[3];
        File.Delete(originalFile + "_backup");
    }

    static internal void DeleteBackup(string originalFile)
    {
        DeleteBackup(new FileInfo(originalFile));
    }
}

I tried my best as I plan to publish source code online but still feel that there is room for improvement.

I guess methods ProcessFile() and IsCorrupted() are not important for that question as well as Logger class.

Important note - StackOverflowException and OutOfMemoryException can't be thrown by this code as it don't do many method calls and don't use much memory.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ How can the file be corrupted mid-processing? Are you updating the file on the disk as you go? Is it an option to simply process the file and only write to disk after processing has completed? Why not simply write to a different file and not touch the original one, to avoid losing the initial data? \$\endgroup\$
    – Flater
    Feb 9 '18 at 12:50
2
\$\begingroup\$

How great is the risk that your files will be corrupted? Is there a particular reason why you think this might happen, and if so, is that cause something you can address?

Are you concerned that it is your act of processing the file that might corrupt it? If so, you may want to do the opposite of what you're doing. Don't create a backup, but instead leave the original alone and create a working copy which you discard when you're done. If something goes wrong then at least your original file is still there, untouched.

This may sound facetious but it's not: If you don't trust your file system then it's not a good place for your application's data. How do you know that even after backing up, the file you're reading won't become corrupt as your're reading it? How do you know it wasn't already corrupt when it was backed up? How do you know that the backups won't be corrupted?

The best approach depends not only on how much you trust (or distrust) your storage but also how great the risk is if your data gets lost. If your server gets backed up regularly then that might be all the protection you need. Perhaps you could create a hash or a checksum for each file and store it with the file. When you process the file, validate that it matches the hash. That gives you about as much certainty as you could possibly want.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you! It was so obvious. I will work with temporary copy of file and apply changes to the original file only if processed version is correct. \$\endgroup\$ Feb 12 '18 at 6:20
2
\$\begingroup\$

I am a little concerned about the static fields used to store timestamps in your BackupHelper class. If someone was using your class in different threads, or backing up more than one at a time, processing them all, then restoring them all, they would potentially end up with incorrect timestamp fields.

One way to resolve this might be to use a Dictionary<string, List<string>>, where the key is the filePath and the timestamps are the value.

Another thing would be to store the string "_backup" as a const. Repeated hard-coded strings can sometimes lead to problems.

\$\endgroup\$
0
1
\$\begingroup\$

All I have for now

I don't like

File.Delete(originalFile + "_backup");

You are passing a FileInfo. I have not tested but I suspect you are getting an implicit cast to string.

FileInfo has a Delete method. Have BackupHelper return a FileInfo so you can delete directly.

\$\endgroup\$
1
  • \$\begingroup\$ The thing is originalFile + "_backup" is an another file. So I decided that this would be a lesser evil. But I actually missed that I pass FileInfo to this method. I definitely should use originalFile.FullName in this case. \$\endgroup\$ Feb 12 '18 at 4:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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