3
\$\begingroup\$

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

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CR! Sounds like the code you're putting up for peer review isn't working as intended, which is drawing close votes (see help center for what's expected of code posted here). If you removed the part about changing what it does (and added more context about what's what.. it's not as clear as you might think), you would be getting extremely valuable feedback on any/all aspects of it; feel free to edit anytime! PS - it's "lambda" ;-) \$\endgroup\$ – Mathieu Guindon Oct 10 at 23:42
  • \$\begingroup\$ This is lambada: en.wikipedia.org/wiki/Lambada \$\endgroup\$ – dfhwze Oct 11 at 4:36
2
\$\begingroup\$
    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();
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for your time and kindness to explain me all of this, i learned a lot . \$\endgroup\$ – Sinesters Oct 13 at 3:46
  • \$\begingroup\$ I only have one question, when i use bitmaps, normally i have to dispose them, so this method dons't increase the memory usage if we keep clicking the random option? \$\endgroup\$ – Sinesters Oct 13 at 4:27
  • \$\begingroup\$ @Sinesters: You have a good point there. In SetImage(), you should check for existing images and call Dispose() on them if present. \$\endgroup\$ – Henrik Hansen Oct 13 at 5:46

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.