2
\$\begingroup\$

Input

  • inputBlob - A PDF or ZIP file containing all the pages of many students' test papers.
  • extension - The file type ("pdf" or "zip").
  • numPages - The number of pages in a single test. For example, if numPages is 2 and the input file contains 10 pages, then we must have 5 students' tests.

Routine

  • Extract all the pages as images.
  • Upload the images to blob storage.
  • For each test (i.e. set of images), queue another routine to process it.

Code

I've created PdfPack and ZipPack classes (implementations not included here) which implement a common interface:

public interface IPack : IDisposable
{
  void Open(Stream stream);
  Stream GetPage(int index);
  int PageCount { get; }
}

Here's the function itself:

public static async Task Run(
  CloudBlockBlob inputBlob,
  string extension,
  int numPages,
  CloudBlobContainer container,
  IAsyncCollector<StitchRequest> queue)
{

  IPack pack = null;

  try {

    using (var inputStream = new MemoryStream()) {
      await inputBlob.DownloadToStreamAsync(inputStream);

      switch (extension.ToLowerInvariant()) {
        case "zip":
          pack = new ZipPack();
          break;
        case "pdf":
          pack = new PdfPack();
          break;
        default:
          throw new InvalidOperationException("Invalid file type.");
      }

      pack.Open(inputStream);

      if (pack.PageCount == 0) {
        throw new InvalidOperationException("No pages found.");
      }

      if (pack.PageCount % numPages != 0) {
        throw new InvalidOperationException($"Pages not a multiple of {numPages}.");
      }

      Guid groupId = default;

      for (var i = 0; i < pack.PageCount; i++) {

        if (i % numPages == 0) {
          groupId = Guid.NewGuid();
        }

        using (Stream imageStream = pack.GetPage(i)) {
          var imageBlob = container.GetBlockBlobReference($"{groupId}/{i % numPages}");
          await imageBlob.UploadFromStreamAsync(imageStream);
        }

        if (i % numPages == numPages - 1) {
          await queue.AddAsync(new StitchRequest {
            GroupId = groupId.ToString("D"),
            Count = numPages
          });
        }

      }
    }

    await inputBlob.DeleteAsync();

  } finally {
    pack?.Dispose();
  }

}
\$\endgroup\$
3
\$\begingroup\$

"Egyptian" brackets is uncommon practice for C#. You should place an opening brace on separate line.


I suggest to use Dictionary every time you deal with mapping:

private const string ZipExtension = "zip";
private const string PdfExtension = "pdf";

private static readonly Dictionary<string, Func<IPack>> PackCreators =
    new Dictionary<string, Func<IPack>>(StringComparer.OrdinalIgnoreCase)
    {
        [ZipExtension] = () => new ZipPack(),
        [PdfExtension] = () => new PdfPack()
    };

Then you can obtain an instance of pack with this code:

if (!PackCreators.TryGetValue(extension, out var packCreator))
    throw new InvalidOperationException("Invalid file type.");

var pack = packCreator();

Once you've got a pack instance you can perform all actions with it inside using block:

using (pack)
{
    pack.Open(inputStream);
    // ...
}

which eliminates the need to define pack outside of try-finally and the need of try-finally itself with explicit pack disposing.


You calculate i % numPages multiple times in your loop. Define a variable and use it:

var remainder = i % numPages;

if (remainder == 0)
// ...
$"{groupId}/{remainder }"
// ...
if (remainder == numPages - 1)
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$
  • Extract all the pages as images.
  • Upload the images to blob storage.
  • For each test (i.e. set of images), queue another routine to process it.

Do you not think that's quite a lot to do inline in one method?


Error handling seems somewhat minimal. The delayed enqueue covers some error cases, but there's still potential for the same batch to be processed multiple times if there's an exception in the last page. Have you considered generating and persisting the GUIDs in one phase so that you can detect this case and avoid reprocessing the parts which were successful?

| improve this answer | |
\$\endgroup\$

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.