Cannot find the mistake in my code it is a calculator that is using a list to store operations and numbers?

Question

Sometimes it works like 2+2*2 = 6 but when 5*5*5=only 25 here s the code dont really know where the mistake is if someone could help couse i am only a begginer i am using 2 lists to store numbers and operations

namespace kalkulator
{
    public partial class Form1 : Form
    {
        List<double> vnos = new List<double>();
        List<char> operacije = new List<char>();

        public Form1()
        {
            InitializeComponent();
            reset();
        }

        private void button_Click(object sender, EventArgs e)
        {
            Button vnos = (Button)sender;
            if (textBox1.Text == "0" && textBox1.Text != null)
                textBox1.Text = vnos.Text;
            else
                textBox1.Text += vnos.Text;
        }

        private void operacija_Click(object sender, EventArgs e)
        {
            try
            {
                vnos.Add(Convert.ToDouble(textBox1.Text));
                vse.Text = vse.Text + textBox1.Text;
                textBox1.Text = "";
                Button button = (Button)sender;
                char gumb = Convert.ToChar(button.Text);
                switch (gumb)
                {
                    case '+':
                        operacije.Add('+');
                        vse.Text = vse.Text + '+';
                        break;
                    case '-':
                        operacije.Add('-');
                        vse.Text = vse.Text + '-';
                        break;
                    case '*':
                        operacije.Add('*');
                        vse.Text = vse.Text + '*';
                        break;
                    case '/':
                        operacije.Add('/');
                        vse.Text = vse.Text + '/';
                        break;
                }
            }
            catch(FormatException)
            {
                MessageBox.Show("Napačni format");
            }
            catch (OverflowException)
            {
                MessageBox.Show("Preveliko število");
            }
            catch(Exception ex)
            {
                MessageBox.Show(ex.Message);
            }
        }

        private void clear_Click(object sender, EventArgs e)
        {
            reset();
        }

        private void reset()
        {
            vse.Text = "";
            textBox1.Text = "";
            vnos.Clear();
            operacije.Clear();
        }

        private void enter_Click(object sender, EventArgs e)
        {
            try
            {
                vnos.Add(Convert.ToDouble(textBox1.Text));
                vse.Text = vse.Text + textBox1.Text;
                textBox1.Text = "";
                vnos.Add(0);
                operacije.Add('.');
                for (int i = 0; i < vnos.Count - 1; i++)
                {
                    if (operacije[i] == '*')
                    {
                        vnos[i] = vnos[i] * vnos[i + 1];
                        vnos.RemoveAt(i + 1);
                        operacije.RemoveAt(i);
                    }
                    else if (operacije[i] == '/')
                    {
                        vnos[i] = vnos[i] / vnos[i + 1];
                        vnos.RemoveAt(i + 1);
                        operacije.RemoveAt(i);
                    }
                }
                for (int i = 0; i < vnos.Count - 1; i++)
                {
                    if (operacije[i] == '+')
                    {
                        vnos[i] = vnos[i] + vnos[i + 1];
                        vnos.RemoveAt(i + 1);
                        operacije.RemoveAt(i);
                    }
                    else if (operacije[i] == '-')
                    {
                        vnos[i] = vnos[i] - vnos[i + 1];
                        vnos.RemoveAt(i + 1);
                        operacije.RemoveAt(i);
                    }
                }
                textBox1.Text = Convert.ToString(vnos[0]);
            }
            catch (FormatException)
            {
                MessageBox.Show("Napačni format");
            }
            catch (OverflowException)
            {
                MessageBox.Show("Preveliko število");
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }
        }

        private void button10_Click(object sender, EventArgs e)
        {
            textBox1.Text = textBox1.Text + ',';
        }
    }
}

Answer 1

Although I agree that this could not be the best way to implement the calculator, I think I found why your code doesn't compute the correct results with what you have at the moment.

In this specific case the issue is related to the way you handle the operators and operands. As this is one of the most common mistakes we make when dealing with loops and collections I'll answer so you can move forward with your code and hopefully learn something in the process. :-)

Take a look at this snippet copied from your enter_Click(object sender, EventArgs e) event handler:

for (int i = 0; i < vnos.Count - 1; i++)
{
    if (operacije[i] == '*')
    {
        vnos[i] = vnos[i] * vnos[i + 1];
        vnos.RemoveAt(i + 1);
        operacije.RemoveAt(i);
    }
    else if (operacije[i] == '/')
    {
        vnos[i] = vnos[i] / vnos[i + 1];
        vnos.RemoveAt(i + 1);
        operacije.RemoveAt(i);
    }
}

The issue with this code is that you're consuming the first token in the operations and the operands arrays and then you're removing them from the array. But the thing is that when you do that, the loop control variable (i) is incremented by the for loop (i++) so you're effectively losing one element to consume.

Let me see if I can illustrate this a little bit:

let's say you clicked the following buttons 5 * 5 * 5 = in that order in your UI. At this point you have:

vnos = [5,5,5,0]
operacije = ['*', '*', '.']

Then you start looping to compute the result. I've removed all the code, left only the relevant bits and written what's happening to your variables step by step below:

for (int i = 0; i < vnos.Count - 1; i++) 
{
    if (operacije[i] == '*')
    {
        vnos[i] = vnos[i] * vnos[i + 1];
        vnos.RemoveAt(i + 1);
        operacije.RemoveAt(i);
    }
}

1. Initialize for loop. i is set to 0.
2. Loop condition gets evaluated 0 < 4 -> true as vnos.Count = 4. Enter loop block
3. Condition if (operacije[i] == '*') is evaluated to true as the first operation is '*'
4. Enter if block
5. The first operand is replaced by the product of itself and the next in the array -> vnos = [25,5,5,0]
6. The next operand is removed from the vnos array -> vnos = [25,5,0]
7. The current operation is removed from the operacije array -> operacije = ['*', '.']
8. Exit the condition block.
9. Loop increment block is executed (i++) -> i = 1. Program flow goes to step #2. But I'll continue here step by step.
10. Loop condition gets evaluated 1 < 3 -> true as vnos.Count = 3. Enter loop block
11. Troubles start here: Condition if (operacije[i] == '*') is evaluated to false because i = 1 and the operation in that index is not '*' but your final delimiter (the dot '.') character.
12. The conditional block is not executed again as it should be and your computation ends.

This is actually one of the most common mistakes we make and the best practice is not to modify a collection when you're looping over it (or be very careful if you do). In fact foreach and iterators don't even allow it.

Actually the solution is not so complex. You just need to decrement the loop control variable after removing the element from the collection. And thus you're tricking the loop because when the increment step is executed, the variable will be pointing at the correct index this time because you manually decremented before.

So for your case, please note that just by adding a decrement operator at the line where you remove the operation from the list is enough. The change is in the line operacije.RemoveAt(i--);.

for (int i = 0; i < vnos.Count - 1; i++) 
{
    if (operacije[i] == '*')
    {
        vnos[i] = vnos[i] * vnos[i + 1];
        vnos.RemoveAt(i + 1);
        operacije.RemoveAt(i--); //<----- i-- instead of i.
    }
}

Please note you must do this change in all your blocks. That is in all the places you remove from the operation list.

Hope this helps!