trjhtr

I have created a program that generates a random image from a folder of all the world's flags. First, it generates an image as it loads, then it can keep randomizing the shown image if the user clicks on a button. Currently, I have created a method called "ImageGenerator" and call that method both in Form1() and my button click method (view code: https://pastebin.com/qzWXqP0P).

public partial class Form1 : Form

 public void ImageGenerator()
 

 string flagImageSources = new string[196];

 StreamReader flagImageSourceReader = File.OpenText("flagImageSourceFile.csv");

 for (int i = 0; i < flagImageSources.Length; i++)
 
 string s = flagImageSourceReader.ReadLine();
 flagImageSources[i] = s;
 

 Random numberGenerator = new Random();
 int imageNumber = numberGenerator.Next(0, 196);

 string flagImageSrc = flagImageSources[imageNumber];

 flagPictureBox.ImageLocation = @flagImageSrc;
 

 public Form1()
 
 InitializeComponent();

 ImageGenerator();
 

 private void submitBtn_Click(object sender, EventArgs e)
 
 ImageGenerator();
 

To me it feels like the linked code is rather inefficient as it recreates the array flagImageSources (which contains the paths to each flag image) every time a new image is to be generated in the picture box. How can I run the code on lines 6 through 13 only once, and then just run the code on lines 16 through 21 when randomizing images?

To answer your immediate question, you need to initialise your flagImageSources once, and store it as a field in the class. That allows you to split the two actions into separate methods - one to get your list of flags, and one to get a random flag.

public partial class Form1 : Form

 private string flagImageSources;

 private void InitializeImageSources()
 
 flagImageSources = new string[196];
 StreamReader flagImageSourceReader = File.OpenText("flagImageSourceFile.csv");

 for (int i = 0; i < flagImageSources.Length; i++)
 
 string s = flagImageSourceReader.ReadLine();
 flagImageSources[i] = s;
 
 

 private string GetRandomImageLocation()
 
 Random numberGenerator = new Random();
 int imageNumber = numberGenerator.Next(0, 196);

 return flagImageSources[imageNumber];
 

But I think we have some other concerns here.

Firstly, you open your file but forget to close it again. C# has a handy way of making sure this doesn't happen - something that gets opened and closed should implement IDisposable (and StreamReader does just that). So you can change your code a little to make use of that, like this:

 using (StreamReader flagImageSourceReader = File.OpenText("flagImageSourceFile.csv"))
 
 //do stuff with flagImageSourceReader
 

Secondly, you're using the magic number of 196 in a couple of places in your code - both to set the initial size of your array, and to set the upper bound of your random number generator. I would suggest that rather than using a fixed-size array, you should use an IList, which is dynamically sized. Then, if North and South Korea should merge, or Scotland or Catalonia or Tibet become independent countries, your code will still work even as you add or remove flags :)

Here is my final code that solves both of those problems:

public partial class Form1 : Form

 private IList<string> flagImageSources = new List<string>();

 private int NumberOfFlags => flagImageSources.Count;

 public Form1()
 
 InitializeComponent();

 InitializeImageSources();
 flagPictureBox.ImageLocation = GetRandomImageLocation();
 

 private void InitializeImageSources()
 
 using (StreamReader flagImageSourceReader = File.OpenText("flagImageSourceFile.csv"))
 
 while (!flagImageSourceReader.EndOfStream)
 
 flagImageSources.Add(flagImageSourceReader.ReadLine());
 
 
 

 private string GetRandomImageLocation()
 
 Random numberGenerator = new Random();
 int imageNumber = numberGenerator.Next(0, NumberOfFlags);

 return flagImageSources[imageNumber];
 

 private void submitBtn_Click(object sender, EventArgs e)
 
 flagPictureBox.ImageLocation = GetRandomImageLocation();