Swap the 1st negative and last positive elements in the matrix. How to fix the error?

Question

C++ / C#

Faced a small problem. How to fix an error when rearranging elements in a matrix? I need to swap the first negative and last positive elements in the output array. When outputting to the 2nd listBox, the result is incorrectly displayed for me. How to fix this problem?

private void button2_Click(object sender, EventArgs e)
        {
            int indexOfNegativeElement = -1;
            int indexOfPositiveElement = -1;

            for (int i = 0; i < 10; i++)
            {
                if (mas[i] < 0)
                {
                    listBox2.Visible = true;
                    indexOfNegativeElement = i;
                    break;
                }

                else
                {
                    label1.Visible = true;
                    
                }
            }

            for (i = 9; i > -1; i--)
            {
                if (mas[i] > 0)
                {
                    listBox2.Visible = true;
                    indexOfPositiveElement = i;
                    break;
                }

                else
                {
                    label1.Visible = true;
                    
                }
            }

            if (indexOfNegativeElement >= 0 && indexOfPositiveElement <= 0)
            {
                int temp = mas[indexOfNegativeElement];
                mas[indexOfNegativeElement] = mas[indexOfPositiveElement];
                mas[indexOfPositiveElement] = temp;
            }
            listBox2.Items.Add(mas[i].ToString());

            if (indexOfNegativeElement <= 0 && indexOfPositiveElement >= 0)
            {
                int temp = mas[indexOfNegativeElement];
                mas[indexOfNegativeElement] = mas[indexOfPositiveElement];
                mas[indexOfPositiveElement] = temp;
            }
            listBox2.Items.Add(mas[i].ToString());
        }

Answer 1

I didn’t go into logic much, but what immediately caught my eye was that the indexOfPositiveElement variable gets an init.value of -1, and then it may not change after any checks. After that, you test it for less than or equal to zero. And in this case, even without the fact that you have found a positive element, you still want to rearrange the values. The same is true for the indexOfNegativeElement variable. I marked both places in bold type.

if (indexOfNegativeElement >= 0 && indexOfPositiveElement <= 0 )
{
int temp = mas[indexOfNegativeElement];
mas[indexOfNegativeElement] = mas[indexOfPositiveElement];
mas[indexOfPositiveElement] = temp;
}
listBox2.Items.Add(mas[i].ToString());
if ( indexOfNegativeElement <= 0 && indexOfPositiveElement >= 0)
{
int temp = mas[indexOfNegativeElement];
mas[indexOfNegativeElement] = mas[indexOfPositiveElement];
mas[indexOfPositiveElement] = temp;
}
listBox2.Items.Add(mas[i].ToString());

In general, I would advise you the names of buttons and sheets, etc. call them what they are. Those. button for example or "btnSwapPairItems", or if not comme il faut without ""btn. This is the first. Second, you have the same repeating action in this function. Take it to a separate function. For example, name it "SwapPairItemsInList" and pass it the parameters of the first and second items to be swapped and call this function every time (twice here) when two items need to be swapped. If this is repeated not only in this window and not only with this list, but in 100 other places, then bring this function into a separate class and increase the parameter of this function with one more - Reference to this control sheet. And that's it - you will never need to fill it with pens again. Pity your time.
After watching, I noticed one more thing. Never just use numbers like you do in your test loops. All these 9 and 10 - today you know why 9 and 10, tomorrow - no. Today 9, tomorrow 900. You need to rewrite all the code. For such things, always use constants, or in this window, if this value plays a role not only in this window, but also in others, display it in a separate class with all the constants on this topic. Call it " public const Int32 MaxItemsInSwapList = 9 ", and use it here in a function and that's it - and after 9 years you know what this 9 means. Or, if 10 or 9 doesn't matter at all, then poll for the number of elements in the list before running through all the values ​​in that list.....but not the numbers.