HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Displaying a new image document in Picturebox

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pictureboximagenewdisplayingdocument

Problem

I'm using WinForms and have a picturebox in my form. This program looks in "C:\image\" directory to find a specified image document, in my case a tif file. There is always only one picture in "C:\image\" directory.

After it locates the file, the program displays the image document in the picturebox.

When I was running this, I saw that the CPU usage was high. My goal is to make performance better or find out if there is a better way of coding this.

I have to manually go into my C:\image\ directory and delete the current image document then place a new image document in there so my picturebox will show the new image document.

int picWidth, picHeight;

private void Form1_Load(object sender, EventArgs e)
{
    timer1_Tick(sender, e);
}

private void File_Length()
{
    try
    {

        string path = @"C:\image\";
        string[] filename = Directory.GetFiles(path, "*.tif"); //gets a specific image doc.

        if (filename.Length > 0)
        {
            FileInfo fi = new FileInfo(filename[0]);
            byte[] buff = new byte[fi.Length];
            using (FileStream fs = File.OpenRead(filename[0]))
            {
                fs.Read(buff, 0, (int)fi.Length);
            }
            MemoryStream ms = new MemoryStream(buff);
            Bitmap img1 = new Bitmap(ms);
            //opened = true; // the files was opened.
            pictureBox1.Image = img1;

            pictureBox1.Width = img1.Width;
            pictureBox1.Height = img1.Height;
            picWidth = pictureBox1.Width;
            picHeight = pictureBox1.Height;
        }
    }
    catch(Exception)
    {

    }
}

public void InitTimer()
{
    timer1 = new Timer(); 
    timer1.Tick += new EventHandler(timer1_Tick); //calls method
    timer1.Interval = 2000; // in miliseconds (1 second = 1000 millisecond)
    timer1.Start(); //starts timer
}

private void timer1_Tick(object sender, EventArgs e)
{
    File_Length(); //checking the file length every 2000 miliseconds
}

Solution

Let's start here:

string[] filename = Directory.GetFiles(path, "*.tif");


If you know there can be only one file then you can try to get it right away with SingleOrDefault

var imageFileName = Directory.GetFiles(path, "*.tif").SingleOrDefault();


the condition will then become

var fileExists = !string.IsNullOrEmpty(imageFileName);
if (!fileExists)
{
   return;
}


Further you don't need all this code

FileInfo fi = new FileInfo(filename[0]);
byte[] buff = new byte[fi.Length];
using (FileStream fs = File.OpenRead(filename[0]))
{
  fs.Read(buff, 0, (int)fi.Length);
}
MemoryStream ms = new MemoryStream(buff);
Bitmap img1 = new Bitmap(ms);
//opened = true; // the files was opened.
pictureBox1.Image = img1;


instead you can just use

pictureBox1.Image = Image.FromFile(imageFileName);


besides there is a memory leak, you need to dispose the old image so before assigning a new one:

if (pictureBox.Image != null)
{
     pictureBox.Image.Dispose();
     pictureBox.Image = null;
}
pictureBox1.Image = Image.FromFile(imageFileName);


About the timer...

time1 = new Timer(); 
time1 .Tick += new EventHandler(timer1_Tick); //calls method
time1 .Interval = 2000; // in miliseconds (1 second = 1000 millisecond)
time1 .Start(); //starts timer


You have too many obvious comments here. There is no need to comment start as start ;-) One of them can even be replaced by a helper variable that makes the comment unnecessary. Meaningful variables are always better then comments.

const int oneSecondInMilisecods = 1000;

time1 = new Timer(); 
time1.Tick += timer1_Tick;
time1.Interval = 2 * oneSecondInMilisecods;
time1.Start();


Notice that you can also use a short event handler assignment.

Other suggestions:

-
You should use full names instead of abbreviations like fi or buff, imageFileInfo and imageBytes sounds much better and you don't have to think first what those variables are for

-
Collections should have plural form so string[] filename should be string[] imageFileNames

-
File_Length is not a good name becuause it doesn't tell us anything about what the method does and in your case it does a completely different thing. LoadImage would be much better here, we also don't put _ in method names unless they are event handlers or tests.

-
I would also change the name of the timer1 to loadImageTimer

-
You didn't put all the image loading code inside the time1_Tick event handler which is good ;-)

Code Snippets

string[] filename = Directory.GetFiles(path, "*.tif");
var imageFileName = Directory.GetFiles(path, "*.tif").SingleOrDefault();
var fileExists = !string.IsNullOrEmpty(imageFileName);
if (!fileExists)
{
   return;
}
FileInfo fi = new FileInfo(filename[0]);
byte[] buff = new byte[fi.Length];
using (FileStream fs = File.OpenRead(filename[0]))
{
  fs.Read(buff, 0, (int)fi.Length);
}
MemoryStream ms = new MemoryStream(buff);
Bitmap img1 = new Bitmap(ms);
//opened = true; // the files was opened.
pictureBox1.Image = img1;
pictureBox1.Image = Image.FromFile(imageFileName);

Context

StackExchange Code Review Q#109922, answer score: 2

Revisions (0)

No revisions yet.