1
\$\begingroup\$

So I have a piece a code that, altough it does what is expected, it doesn't seam very performant to me. I just wanted to know if there's a better way to do this or if my code is alright as it is.

The objective of the code is to recieve an image path, check if the image exists or if it isn't png, if the image isn't png convert it to png and return a byte[] of the stream.

the code:

    // the speed of the method, on my computer, as is, right now, is of 40ms per cycle, this means 10 cycles are around 400ms
    public static byte[] GetBytesFromImage(string imageName)
    {
        byte[] bytes = null;

        string imagePath = "..\\..\\Imagens";
        imageName = imageName == "" ? "\\logotipos-prototipo_logo_prototipo_print.jpg" : imageName;
        string fpath = imagePath + imageName;

            if (File.Exists(fpath) && !imageName.EndsWith("png"))
            {
                using (MemoryStream image = new MemoryStream())
                using (System.Drawing.Image bmpImageToConvert = System.Drawing.Image.FromFile(fpath))
                using (System.Drawing.Image bmpNewImage = new Bitmap(bmpImageToConvert.Width, bmpImageToConvert.Height))
                using (Graphics gfxNewImage = Graphics.FromImage(bmpNewImage))
                {
                    gfxNewImage.DrawImage(bmpImageToConvert, new Rectangle(0, 0, bmpNewImage.Width, bmpNewImage.Height), 0, 0, bmpImageToConvert.Width, bmpImageToConvert.Height, GraphicsUnit.Pixel);

                    bmpNewImage.Save(image, ImageFormat.Png);
                    bytes = image.ToArray();
                }
            }

        return bytes;
    }

1st Update: As the @Nkosi's answer mentioned, I forgot to make a case for when the image is PNG, and also to deal with the method returning a potencial null byte[].
Also in his change now the MemoryStream is shared in both cases, and the string comparison now ignores the string case.

    readonly string BASEIMAGEPATH = "..\\..\\Imagens\\";
    readonly string DEFAULTIMAGENAME = "logotipos-prototipo_logo_prototipo_print.jpg";


    // the speed of the method, on my computer, as is, right now, is of 40ms per cycle, this means 10 cycles are around 400ms
    public static byte[] GetBytesFromImage(string imageName)
    {
        byte[] bytes = null;
        imageName = imageName == "" ? DEFAULTIMAGENAME : imageName;
        string path = Path.Combine(BASEIMAGEPATH, imageName);
        if (File.Exists(path))
        {
            // Now both cases use the same MemoryStream
            using (MemoryStream image = new MemoryStream())
            {
                // The comparison now ignores case
                if (imageName.EndsWith("png", StringComparison.InvariantCultureIgnoreCase))
                {
                    using (FileStream imageStream = new FileStream(path, FileMode.Open, FileAccess.Read))
                    {
                        imageStream.CopyTo(image);
                        bytes = image.ToArray();
                    }
                }
                else
                {
                    using (System.Drawing.Image bmpImageToConvert = System.Drawing.Image.FromFile(path))
                    using (System.Drawing.Image bmpNewImage = new Bitmap(bmpImageToConvert.Width, bmpImageToConvert.Height))
                    using (Graphics gfxNewImage = Graphics.FromImage(bmpNewImage))
                    {
                        gfxNewImage.DrawImage(bmpImageToConvert, new Rectangle(0, 0, bmpNewImage.Width, bmpNewImage.Height), 0, 0, bmpImageToConvert.Width, bmpImageToConvert.Height, GraphicsUnit.Pixel);
                        bmpNewImage.Save(image, ImageFormat.Png);
                        bytes = image.ToArray();
                    }
                }
            }
        }
        // I changed his code here because `Array.Empty<byte>()` didn't exist.
        return bytes ?? new byte[0];
    }
\$\endgroup\$
5
  • \$\begingroup\$ Why the for loop? \$\endgroup\$ – Nkosi Dec 30 '19 at 12:18
  • \$\begingroup\$ that was just adde to test various iterations of that part of the method, it isn't doing nothing functionally \$\endgroup\$ – Erick Santander Dec 30 '19 at 12:19
  • 1
    \$\begingroup\$ I edited it, so that the for loop is no longer there. \$\endgroup\$ – Erick Santander Dec 30 '19 at 12:29
  • \$\begingroup\$ Please don't change your code after receiving an answer, as it invalidates the answer. You can always ask a new question using updated code. \$\endgroup\$ – Cris Luengo Dec 30 '19 at 19:42
  • \$\begingroup\$ Oh! sorry, I can make an Update and distinguish both, or leave the old code. \$\endgroup\$ – Erick Santander Dec 31 '19 at 9:14
3
\$\begingroup\$

There is a lot of unnecessary noise in the function.

If the base path is not expected to change it can be moved out into a constant.

System.IO.Path can be used to construct the path to the desired image.

To avoid null reference errors when consuming this function, try to avoid returning null. An empty array should be returned.

const string BASEIMAGEPATH = "..\\..\\Imagens";
const string DEFAULTIMAGENAME = "\\logotipos-prototipo_logo_prototipo_print.jpg";

public static byte[] GetBytesFromImage(string imageName) {
    byte[] bytes = null;
    imageName = imageName == "" ? DEFAULTIMAGENAME : imageName;
    string path = Path.Combine(BASEIMAGEPATH, imageName);
    if (File.Exists(path) && !imageName.EndsWith("png", StringComparison.InvariantCultureIgnoreCase)) {
        using (MemoryStream image = new MemoryStream())
        using (System.Drawing.Image bmpImageToConvert = System.Drawing.Image.FromFile(path))
        using (System.Drawing.Image bmpNewImage = new Bitmap(bmpImageToConvert.Width, bmpImageToConvert.Height))
        using (Graphics gfxNewImage = Graphics.FromImage(bmpNewImage)) {
            gfxNewImage.DrawImage(bmpImageToConvert, new Rectangle(0, 0, bmpNewImage.Width, bmpNewImage.Height), 0, 0, bmpImageToConvert.Width, bmpImageToConvert.Height, GraphicsUnit.Pixel);
            bmpNewImage.Save(image, ImageFormat.Png);
            bytes = image.ToArray();
        }
    }
    return bytes ?? Array.Empty<byte>();
}

The logic should also be reviewed as it was stated

The objective of the code is to receive an image path, check if the image exists or if it isn't png, if the image isn't png convert it to png and return a byte[] of the stream.

but currently, if the image exists and is already a PNG, it wont return the contents of the file.

const string BASEIMAGEPATH = "..\\..\\Imagens";
const string DEFAULTIMAGENAME = "\\logotipos-prototipo_logo_prototipo_print.jpg";

public static byte[] GetBytesFromImage(string imageName) {
    byte[] bytes = null;
    imageName = imageName == "" ? DEFAULTIMAGENAME : imageName;
    string path = Path.Combine(BASEIMAGEPATH, imageName);
    if (File.Exists(path)) {
        using (MemoryStream image = new MemoryStream()) {
            if (imageName.EndsWith("png", StringComparison.InvariantCultureIgnoreCase)) {
                using (FileStream imageStream = new FileStream(path, FileMode.Open, FileAccess.Read)) {
                    imageStream.CopyTo(image);
                    bytes = image.ToArray();
                }
            } else {
                using (System.Drawing.Image bmpImageToConvert = System.Drawing.Image.FromFile(path))
                using (System.Drawing.Image bmpNewImage = new Bitmap(bmpImageToConvert.Width, bmpImageToConvert.Height))
                using (Graphics gfxNewImage = Graphics.FromImage(bmpNewImage)) {
                    gfxNewImage.DrawImage(bmpImageToConvert, new Rectangle(0, 0, bmpNewImage.Width, bmpNewImage.Height), 0, 0, bmpImageToConvert.Width, bmpImageToConvert.Height, GraphicsUnit.Pixel);
                    bmpNewImage.Save(image, ImageFormat.Png);
                    bytes = image.ToArray();
                }
            }
        }
    }
    return bytes ?? Array.Empty<byte>();
}
\$\endgroup\$
1
  • \$\begingroup\$ thanks for your reply, you certainly have a point there, I hadn't considered that if the image is a PNG then it wont return the array. As for the path, I just put a generic path to help visualize the method. \$\endgroup\$ – Erick Santander Dec 30 '19 at 12:42

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.