Implement DRY principle with IAsyncDisposable

Question

This is a tiny class that creates backup copies of a file so these can be diff'ed to spot the changes compared to last run; used when generating code and so far has proved to be very helpful.

Basically, I reuse the async disposable logic because it doesn't really make sense to write it twice; the documentation simply doesn't bother about that so it's more or less an improvisation.

Example:

await using (var history = await FileHistory.CreateAsync("test.cs"))
{
    if (history != null)
    {
        var path = history.Info.FullName;
        var text = DateTime.Now.ToString(CultureInfo.InvariantCulture);
        await File.WriteAllTextAsync(path, text);
    }
}

The first time it is run:

• user writes test.cs
• nothing else happens as there's no history yet

The second time it is run:

• user writes test.cs
• writes previous version of test.cs to test-backup.cs

The third time it is run:

• user writes test.cs
• writes previous version of test-backup.cs to test-backup-2022-12-31T22-29-28-8881210Z.cs
• writes previous version of test.cs to test-backup.cs

And so on, it creates backup copies of the file which can then be compared for changes.

Code:

using JetBrains.Annotations;

namespace abcd;

public sealed class FileHistory : IDisposable, IAsyncDisposable
{
    private FileHistory(FileInfo info, byte[] data)
    {
        Info = info;
        Data = data;
    }

    [PublicAPI]
    public FileInfo Info { get; }

    private byte[] Data { get; }

    private bool IsDisposed { get; set; }

    public async ValueTask DisposeAsync()
    {
        await DisposeAsyncCore(false).ConfigureAwait(false);

        GC.SuppressFinalize(this);
    }

    public void Dispose()
    {
        Dispose(true);

        GC.SuppressFinalize(this);
    }

    ~FileHistory()
    {
        Dispose(false);
    }

    private async ValueTask DisposeAsyncCore(bool disposing)
    {
        if (IsDisposed)
            return;

        if (disposing)
        {
            // NOP
        }

        if (Data.Length is not 0)
        {
            Info.Refresh();

            if (Info.Exists)
            {
                var name = Info.FullName;

                var data = await File.ReadAllBytesAsync(name);

                if (data.AsSpan().SequenceEqual(Data) is false)
                {
                    var type = Path.GetExtension(name);

                    name = name[..^type.Length];

                    var path = $"{name}-backup{type}";

                    if (File.Exists(path))
                    {
                        var time = File.GetLastWriteTimeUtc(path);
                        var dest = $"{name}-backup-{time.ToString("O").Replace(':', '-').Replace('.', '-')}{type}";
                        File.Move(path, dest);
                    }

                    await File.WriteAllBytesAsync(path, Data);
                }
            }
        }

        IsDisposed = true;
    }

    private void Dispose(bool disposing)
    {
        DisposeAsyncCore(disposing).AsTask().Wait();
    }

    public static async Task<FileHistory?> CreateAsync(string path, CancellationToken cancellationToken = default)
    {
        var info = new FileInfo(path);

        byte[] data;

        if (info.Exists)
        {
            await using var stream = info.OpenRead();

            data = new byte[stream.Length];

            // ReSharper disable once UnusedVariable

            var read = await stream.ReadAsync(data, cancellationToken);
        }
        else
        {
            data = Array.Empty<byte>();
        }

        return cancellationToken.IsCancellationRequested ? null : new FileHistory(info, data);
    }
}

Answer 1

I read your code three times and quite frankly it does not make too much sense for me.
(Maybe I'm just tired, but still it doesn't ...)

• You have implemented the disposable pattern where your are not freeing up any resources
  • The FileHistory does not even own any resources at all
• It seems like the whole implementation is aiming for small file
  • which can nicely fit into a pre-allocated byte array
  • but it would miserably fail with large files
• you are reading the entire file again and compare its content byte-wise
  • why checking the last modification date is not enough?
• ToString("O").Replace(':', '-').Replace('.', '-')
  • This date formatting logic is ... well sub-optimal
• Sometimes you try to follow good async practices like: .ConfigureAwait(false)
  • Where other times you don't: .AsTask().Wait()

---

Apologize if my post felt as offensive that was not my intent.

---

UPDATE #1

As for the date formatting, I don't know, maybe you can suggest better.

Your chosen date time format (2022-12-31T22-29-28-8881210Z) is not supported out of the box by any formatter. Mostly because the hyphens are usually used only on the date part, but not on the time component.

So, using o or s formatter changes only the date part:

o: 2023-01-03T08:23:27.1870000Z
s: 2023-01-03T08:23:27

There is a class called DateTimeFormatInfo which has a DateSeparator and a TimeSeparator. You can set both to hyphens.

CultureInfo culture = CultureInfo.CreateSpecificCulture("en-US");
DateTimeFormatInfo dtfi = culture.DateTimeFormat;
dtfi.DateSeparator = "-";
dtfi.TimeSeparator = "-";

The problem with this approach is that the o or s formatter does not take advantage of the TimeSeparator :(

G: 1-3-2023 8-28-52 AM
O: 2023-01-03T08:28:52.8980000Z
s: 2023-01-03T08:28:52
T: 8-28-52 AM
u: 2023-01-03 08:28:52Z
U: Tuesday, January 3, 2023 8-28-52 AM

You can set the FullDateTimePattern on the DateTimeFormatInfo to specify how the U formatter should work

dtfi.FullDateTimePattern = "yyyy-MM-ddThh-mm-ss-ffffffZ";
//2023-01-03T08-34-30-538000Z

But this is basically the same as providing the above pattern to the ToString, which is my recommendation:

ToString("yyyy-MM-ddThh-mm-ss-ffffffZ")

---

UPDATE #2

Finally, AsTask().Wait() is the simplest way I found to apply DRY, I don't like it very much, hence why I asked for a code review :)

There are multiple ways to solve to address this:

• At very least prefer .GetAwaiter().GetResult() over .Wait()
• It is not mandatory to implement both interfaces
  • But if you do then you can perform casting to call the proper Dispose or DisposeAsync method on a given resource
  • Since FileHistory does not own any resources this suggestion is not applicable for your code
• Since you have stated that this design is meant for small files that's why using only the synchronous file operations might be fine

var data = File.ReadAllBytes(name);
...
File.WriteAllBytes(path, Data);
...
return ValueTask.CompletedTask;

• And last but not least you can do some branching inside the DisposeAsyncCore.
  • It feels like a bit dirty but it does its job correctly

var data = isSync
    ? File.ReadAllBytes(name)
    : await File.ReadAllBytesAsync(name);
...
if (isSync) File.WriteAllBytes(path, Data);
else await File.WriteAllBytesAsync(path, Data);