6
\$\begingroup\$

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
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

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 ;-)

\$\endgroup\$
1
\$\begingroup\$

The high CPU usage is probably because you are checking checking the folder and loading the image every 2 seconds. Instead of using a timer, there's a FileSystemWatcher class that allows you to handle events if the files in a specified directory have been changed, renamed, etc.

For your needs, you could do something like:

        FileSystemWatcher watcher = new FileSystemWatcher();
        watcher.Path = @"C:\image\";

        watcher.NotifyFilter = NotifyFilters.LastAccess | NotifyFilters.LastWrite
           | NotifyFilters.FileName | NotifyFilters.DirectoryName;

        watcher.Filter = "*.tif";

        watcher.Changed += new FileSystemEventHandler(File_Length);
        watcher.Created += new FileSystemEventHandler(File_Length);
        watcher.Deleted += new FileSystemEventHandler(File_Length);
        watcher.Renamed += new RenamedEventHandler(File_Length);

        watcher.EnableRaisingEvents = true;

This way, the picture is refreshed only if needed.

\$\endgroup\$

Your Answer

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

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