While loop doesn't stop after Boolean is changed

Question

I am working with a small experiment project with windows form application and I got a problem with while loop, after button click. After I click button2 the boolean b should be changed to false and the loop should stop but it isn't.

namespace Spalvos
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }
        Boolean b;
        private void button1_Click(object sender, EventArgs e)
        {
            b = true;
            while (b == true) {
                Random rnd = new Random();
                int r = rnd.Next(0, 254);
                int n = rnd.Next(0, 254);
                int d = rnd.Next(0, 254);

                this.BackColor = Color.FromArgb(r, n, d);
                Application.DoEvents();
                System.Threading.Thread.Sleep(200);              
            }
        }

        private void button2_Click(object sender, EventArgs e)
        {
            b = false;
        }
    }
}

Possible duplicate of [Problems of while(true) in C# "VS2012" {WinForm}](https://stackoverflow.com/questions/31896721/problems-of-whiletrue-in-c-sharp-vs2012-winform) — gunr2171, Aug 16 '18 at 15:54
Don't use `DoEvents`. It's a hangover from the VB days. If you want to do something in the 'background', either start a thread, or use a timer. — Neil, Aug 16 '18 at 15:54
Or a duplicate of https://stackoverflow.com/questions/27580241/breaking-from-a-loop-with-button-click-c-sharp — gunr2171, Aug 16 '18 at 15:55
You should not initialize `rnd` inside the loop like that, since it uses the system clock to create a default seed value. Instead you should just declare it at class scope and initialize it once for the best random results. — Rufus L, Aug 16 '18 at 15:56
A variable used in this fashion should probably be [marked volatile](https://stackoverflow.com/questions/72275/when-should-the-volatile-keyword-be-used-in-c). — John Wu, Aug 16 '18 at 16:00
I click button1 and color loop starts but after I click button2 it does'nt stop(it should stop) — D0mm, Aug 16 '18 at 16:01
@JohnWu From your link: "I discourage you from ever making a volatile field" So why do you think that applies here, especially since I don't seen indication of multithreading in the OP? — Kenneth K., Aug 16 '18 at 16:31
@KennethK. Because of potential compiler optimizations that would preclude the Button1_Click handler from seeing any changes made from outside the immediate code block. — John Wu, Aug 16 '18 at 16:36
@JohnWu Again, there's no indication of multi-threading here, so why would that apply? Everything that I'm finding on the web points to `volatile` being important in multi-threaded apps. Even [the documentation](https://learn.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2010/x13ttww7(v=vs.100)) states "volatile keyword indicates that a field might be modified by multiple threads that are executing at the same time". Event handlers aren't (by default) multi-threaded in WinForms. — Kenneth K., Aug 16 '18 at 17:00

score 3 · Answer 1 · answered Aug 16 '18 at 16:25

Your code works but it's a little flaky; events from Button2 sometimes get lost or delayed, possibly because of the way you are intermingling calls to Sleep() and DoEvents, and use of DoEvents is highly discouraged, and superannuated by the introduction of async and await. Also, you should only initialize your Random one time (unless you are doing something really weird).

You can get a much better and more modern solution if you make your click handler async, like so:

    private volatile Boolean b;

    private async void button1_Click(object sender, EventArgs e)
    {
        Random rnd = new Random();

        b = true;
        while (b)
        {
            int r = rnd.Next(0, 254);
            int n = rnd.Next(0, 254);
            int d = rnd.Next(0, 254);

            this.BackColor = Color.FromArgb(r, n, d);
            await Task.Delay(200);
        }
    }

    private void button2_Click(object sender, EventArgs e)
    {
        b = false;
    }
}

This solution has its problems too, however. Imagine what would happen if you clicked Button1 twice, for example. Probably not what was intended.

I would suggest you move the logic to change colors to a timer control, and use the Button1 event handler to enable it and the Button2 event handler to disable it.

public Form1()
{
    InitializeComponent();
    this.timer1.Interval = 200;
}

private Random rnd = new Random();

private void button1_Click(object sender, EventArgs e)
{
    this.timer1.Enabled = true;
}

private void button2_Click(object sender, EventArgs e)
{
    this.timer1.Enabled = false;
}

private void timer1_Tick(object sender, EventArgs e)
{
    int r = rnd.Next(0, 254);
    int n = rnd.Next(0, 254);
    int d = rnd.Next(0, 254);

    this.BackColor = Color.FromArgb(r, n, d);
}

Isn't that much simpler?

Pawel Wujczyk · Answer 2 · 2018-08-28T18:38:30.107

In your code you are invoking Thread.Sleep method on the UI thread. This will result in blocking the UI. You could check it by expanding the millisecond value in the Thread.Sleep.

To make UI responsive, you using hack Application.DoEvents(). Which stops all other threads and invokes all methods which are waiting (in this example are blocked by the loop). Without it after running the application, your form will be frozen and you won't be able to click button or move the window.

This is not correct way of writing the code. Application should do the most consuming work in the background and only switch for short time to perform operation on the UI.

Good way is to start while loop in the background, calculate color in the background and use delegate to switch to UI when needed.

delegate void ChangeColorDelegate(Color c);
public partial class Form1 : Form
{
    Thread thread;
    Boolean b;
    Random Random;

    public Form1()
    {
        InitializeComponent();
        thread = new Thread(ChangeColorBackground);
        thread.Start();
        Random = new Random();
    }

    private void button1_Click(object sender, EventArgs e)
    {
        b = true;
    }

    private void button2_Click(object sender, EventArgs e)
    {
        b = false;
    }

    private void ChangeColorBackground()
    {
        while (true)
        {
            if (b == true)
            {
                int r = Random.Next(0, 254);
                int n = Random.Next(0, 254);
                int d = Random.Next(0, 254);
                Color c = Color.FromArgb(r, n, d);
                ChangeColorThreadSafe(c);
            }
            System.Threading.Thread.Sleep(100);
        }
    }

    private void ChangeColorThreadSafe(Color c)
    {
        if (button1.InvokeRequired)
        {
            ChangeColorDelegate delgate = new ChangeColorDelegate(ChangeColorThreadSafe);
            button1.Invoke(delgate, new object[] { c });
        }
        else
        {
            this.BackColor = c;
        }
    }
}

I created the example on github.

`Random rnd = new Random();` should never be inside a loop. – LarsTech Aug 16 '18 at 20:26 — LarsTech, Aug 16 '18 at 20:26

While loop doesn't stop after Boolean is changed

2 Answers2