Simple winform that randomizes images in a picturebox

Question

I wrote this code for a simple app that gives us random images. The code works, but I feel like I'm doing something wrong. And one thing I would love to know is instead of grabbing from a folder called "resources", we would actually grab from a custom folder inside my exe. So there's no need to add any folder to a different computer, all works inside the exe.

• Is that possible to do ?

I'm slowly learning C#. Thank you for your time in reading my post.

^{P.S I'm very new in using the Lambda expression.}

 public partial class Menu : Form
{
    private string[] files;
    private string[] files2;
    private string[] files3;
    private string[] files4;
    private int currentIndex = 0;
    private int currentIndex2 = 0;
    private int currentIndex3 = 0;
    private int currentIndex4 = 0;
    Random rnd = new Random();

    public Menu()
    {
        InitializeComponent();
    }

    private void initializeImages()
    {
        string appRoot = System.IO.Path.GetDirectoryName(Application.ExecutablePath);
        files = System.IO.Directory.GetFiles(appRoot + @"\Resources");
        files2 = System.IO.Directory.GetFiles(appRoot + @"\Resources");
        files3 = System.IO.Directory.GetFiles(appRoot + @"\Resources");
        files4 = System.IO.Directory.GetFiles(appRoot + @"\Resources");
        files = files.OrderBy(x => rnd.Next()).ToArray();
        files2 = files.OrderBy(x => rnd.Next()).ToArray();
        files3 = files.OrderBy(x => rnd.Next()).ToArray();
        files4 = files.OrderBy(x => rnd.Next()).ToArray();
    }

    private void setImage()
    {
        pictureBox1.ImageLocation = files[currentIndex];
        pictureBox2.ImageLocation = files2[currentIndex2];
        pictureBox3.ImageLocation = files3[currentIndex3];
        pictureBox4.ImageLocation = files4[currentIndex4];
    }

    private void nextImage()
    {
        currentIndex = currentIndex < files.Length - 1 ? currentIndex + 1 : files.Length - 1;
        setImage();
    }

    private void button1_Click(object sender, EventArgs e)
    {
        initializeImages();
        nextImage();
    }

}

Answer 1

    private void initializeImages()
    {
        string appRoot = System.IO.Path.GetDirectoryName(Application.ExecutablePath);
        files = System.IO.Directory.GetFiles(appRoot + @"\Resources");
        files2 = System.IO.Directory.GetFiles(appRoot + @"\Resources");
        files3 = System.IO.Directory.GetFiles(appRoot + @"\Resources");
        files4 = System.IO.Directory.GetFiles(appRoot + @"\Resources");
        files = files.OrderBy(x => rnd.Next()).ToArray();
        files2 = files.OrderBy(x => rnd.Next()).ToArray();
        files3 = files.OrderBy(x => rnd.Next()).ToArray();
        files4 = files.OrderBy(x => rnd.Next()).ToArray();
    }

Here you load the same file names into four different arrays of strings. This is very memory inefficient and unnecessary. Instead only load them once into a single array and shuffle that when you want to change the images in the picture boxes.

In general: when you feel the need to add indices to the names of a set of variables like you do, you should reconsider your design - and you'll probably realize that an array or list or other kind of collection is the solution.

---

Random rnd = new Random();

In general: avoid abbreviated names of properties, fields, methods, object/classes etc. Provide descriptive names and let intellisense in the IDE do the work for you. Here random or randomGenerator would be descriptive names.

---

private void button1_Click(object sender, EventArgs e)
    {
        initializeImages();
        nextImage();
    }

Here you repeatedly reload the image file names. This makes sense if the number of images in the location changes - but you look in a sub folder to the installation folder which indicates that they are rather constant, so no need to do it other than when the form loads.

---

Below I've tried to refactor your code while implementing the above suggestions:

private string[] imageFileNames;
Random random = new Random();

public Menu()
{
  InitializeComponent();

  InitializeImages();
}

private void InitializeImages()
{
  imageFileNames = Directory.GetFiles(Path.Combine(Path.GetDirectoryName(Application.ExecutablePath), "Resources"));
}

private void ShuffleImages()
{
  Array.Sort(imageFileNames, (a, b) => random.Next(0, 3) - 1);
}

private void SetImages()
{
  var picturBoxes = new[] { pictureBox1, pictureBox2, pictureBox3, pictureBox4 };

  for (int i = 0; i < picturBoxes.Length; i++)
  {
    picturBoxes[i].ImageLocation = imageFileNames[i % imageFileNames.Length];
  }
}

private void button1_Click(object sender, EventArgs e)
{
  ShuffleImages();
  SetImages();
}

---

If the number of images is limited you can consider to load them all once into an list of Image objects and then shuffle these instead of the file names.

private void InitializeImages()
{
  images = Directory
    .GetFiles(Path.Combine(Path.GetDirectoryName(Application.ExecutablePath), "Resources"))
    .Select(fn => Image.FromFile(fn)).ToArray();
}