1
\$\begingroup\$

I have a method that on click produces an excel file with information in it. I have been asked to pull images from the data base and add them within the excel file as well. This is how I am pulling the images:

 ProductImages = new List<ProductImageAndMeta>();
                var flyerBlocks = await FlyerBlockRepository.FindAllInAllPageByFLyerIdAsync(selectedFlyer.Id);
                var ComposerDataImages = await ComposerDataFilesRepo.GetAllFlyerImages(FlyerId);

                foreach (var ComposerDataImage in ComposerDataImages)
                {
                    var ProductImage = new ProductImageAndMeta();
                    ProductImage.Id = ObjectId.Parse(ComposerDataImage.Id);
                    Stream stream = await ComposerDataFilesRepo.OpenDataAsync(ComposerDataImage.Filename);
                    ProductImage.Image = await BaseBarParser.ConvertToMemoryStram(stream);
                    ProductImage.Filename = ComposerDataImage.Filename;
                    ProductImage.metadata = ComposerDataImage.metadata;
                    ProductImages.Add(ProductImage);
                }

This is my GetAllFlyerImages method:

   public async Task<IEnumerable<ComposerDataFiles>> GetAllFlyerImages(string FlyerId)
        {
            //var filter = Builders<ComposerDataFiles>.Filter.Empty;
            //return (await Collection.FindAsync(filter)).ToList();
            var filter = Builders<GridFSFileInfo>.Filter.Where(f => f.Metadata["flyerId"] == FlyerId && f.Metadata["type"] == "ProductImage");
            var dataRecords = await FindDataAsync(filter);

            var results = dataRecords.ToList();
            List<ComposerDataFiles> images = new List<ComposerDataFiles>();

            foreach (var result in results)
            {
                var image = new ComposerDataFiles();
                image.Id = result.Id.ToString();
                image.Filename = result.Filename;
                image.metadata = BsonSerializer.Deserialize<ComposerDataFilesMetadata>(result.Metadata);
                images.Add(image);
            }
            return images;
        }

This is my Open DataAsyncMethod:

  public async Task<GridFSDownloadStream> OpenDataAsync(string filename)
        {
            var file = await FindOneDataAsync(filename);
            if (file == null)
                return null;
            return await _bucket.OpenDownloadStreamAsync(file.Id);
        }

ProductImageAndMetaClass:

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Threading.Tasks;

namespace CTCDigital.InDesign.Models.Flyer
{
    public class ProductImageAndMeta
    {
        public MongoDB.Bson.ObjectId? Id { get; set; }
        public string Filename { get; set; }
        public MemoryStream Image { get; set; }
        public ComposerDataFilesMetadata metadata { get; set; }
    }
}

I am pretty sure where my slowdown occurs is the foreach being used to add one product image at a time. I can't seem to wrap my head around how to make it quicker though. Any help would be appreciated.

\$\endgroup\$
1
  • \$\begingroup\$ flyerBlocks is not used in the current in the current code. did you remove the related code ? \$\endgroup\$
    – iSR5
    Commented Jul 21, 2022 at 2:26

1 Answer 1

4
\$\begingroup\$

Some quick remarks:

  • Your code uses inconsistent capitalization, which often does not follow Microsoft's guidelines.

  • Don't needlessly abbreviate: why use "repo" in ComposerDataFilesRepo instead of "repository"?

  • The whole foreach (var ComposerDataImage in ComposerDataImages) block could be done as a .Select() on ComposerDataImages by using a second method that accepts a ComposerDataImage as a param and returns a ProductImage. Same with the foreach (var result in results) code, BTW.

  • var dataRecords = await FindDataAsync(filter); and var results = dataRecords.ToList(); can be done in a single line, plus then you don't need to then use the nondescriptive "results".

  • A class name should almost never be a plural: ComposerDataFiles.

  • GetAllFlyerImages returns IEnumerable<ComposerDataFiles> which get called ComposerDataImages in your first code block. To me, this is confusing. Is it a FlyerImage? A ComposerDataFile? A ComposerDataImage?

  • Why is it called ProductImageAndMeta? Is there already a ProductImage class?

Honestly, this code feels unpolished and rough.


As for the performance: yes, having to open a stream for each image etc. can become expensive, but it is nearly impossible for us to know where the bottleneck is. How many images are involved? How big is each image? How are they stored? Etc.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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