Problem
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];
}
Solution
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>();
}