Feels like too many If's!

I am developing an app for a mobile device and it feels like I am using way too many If / else statements.

The scenario is I have a number of picture boxes which change colour on being clicked in. I check the coolour of each picture box via the colour whether or not a number is stored into an int array.

i.e:

for (int i = 0; i < 6; i++)
{
if (PictureBox1.BackColor == Color.Red)
{
Number[i] = 1;
}
else if (PictureBox2.BackColor == Color.Red)
{
Number[i] = 2;
}
else if (PictureBox3.BackColor == Color.Red)
{
Number[i] = 3;
}
else if (PictureBox4.BackColor == Color.Red)
{
Number[i] = 4;
}
}


Is there any other way I could go about this? or is this my only bet?

Thanks.

P.S - the if list is a lot bigger than what is pasted here :)

P.S - I just noticed my code will not work properly, and simply flood the array with the first value it goes too.. hehe oops.. either way the question still remains. Thanks.

What do You want to think of doing here please explain us better...

The first, second third..... even the last will not work at all :( :p
Microfone:>>>> please try a dynamic collection of pictures to see what is going to happen -

Oppp: whispering: is this feature allowable in C# ? :(:

Quote:

---

Originally Posted by demise

I check the coolour of each picture box via the colour whether or not a number is stored into an int array.

---

:ehh: do you really understand your code?

I have no idea what you are going to do. to me it looks like that you are setting the value in the "Number" array accordingly to the BackColor of you picture boxes.

@demise: is it really so hard to use the code tags?

Code:

---

for (int i = 0; i < 6; i++)
{
        if (PictureBox1.BackColor == Color.Red)
        {
                Number[i] = 1;
        }
        else if (PictureBox2.BackColor == Color.Red)
        {
                Number[i] = 2;
        }
        else if (PictureBox3.BackColor == Color.Red)
        {
                Number[i] = 3;
        }
        else if (PictureBox4.BackColor == Color.Red)
        {
                Number[i] = 4;
        }
}

---

The code does the following:

There is a number of picture boxes, when some are selected they will change their back colour to red. Then I need to know the positions of which were changed to red. So the following code would look to see if seperately each picturebox does have the red colour. If it does, it stores it's number (position) into the number array.

Note: Yes, I know this does not work, I posted it very late. It does work however when I amend it as so:

Code:

---

for (int i = 0; i < 6; i++)
{
        if (PictureBox1.BackColor == Color.Red)
        {
                Number[i] = 1;
                PictureBox1.BackColor = Color.Black;
        }
        else if (PictureBox2.BackColor == Color.Red)
        {
                Number[i] = 2;
                PictureBox2.BackColor = Color.Black;
        }
        else if (PictureBox3.BackColor == Color.Red)
        {
                Number[i] = 3;
                PictureBox3.BackColor = Color.Black;
        }
        else if (PictureBox4.BackColor == Color.Red)
        {
                Number[i] = 4;
                PictureBox4.BackColor = Color.Black;
        }
}

---

I just feel the method is messy.. hence asking for a better solution and advice in any form.

Thanks.

Quote:

---

Originally Posted by demise

There is a number of picture boxes, when some are selected they will change their back colour to red.

---

ok, so you handle some event to change the back color to red?

Quote:

---

Originally Posted by demise

Then I need to know the positions of which were changed to red.

---

I don't understand this part. do you just want to know which picture box is selected (red) ?

Quote:

---

Originally Posted by memeloo

ok, so you handle some event to change the back color to red?

---

Yes, there is a click event, which will change the backcolour to red if it is black. Or black if it is red.

Quote:

---

Originally Posted by memeloo

I don't understand this part. do you just want to know which picture box is selected (red) ?

---

Yes, I want to know which picture boxes are selected. For example:

pb1 pb2 pb3
pb4 pb5 pb6

Assuming the above are picture boxes.

If pb1 and pb6 were red, I would want to remember these picture boxes were red for a future use. Therefore I want to create the number array and store which are red. In this case, pb1 and pb6 (1 and 6).

here's my suggestion:

Code:

---

// create a helper list
HashSet<PictureBox> selectedPictureBoxes = new HashSet<PictureBox>();

// and update it in your selected event handler
void your_picturebox_selected_eventhandler(...)
{
        if(PictureBox1.BackColor == Color.Red)
                selectedPictureBoxes.Add(PictureBox1)
        else
                selectedPictureBoxes.Remove(PictureBox1);
}

---

then you can use it somewhere else with the "for each" loop

That seems like a good idea :) I will give that a go. Thank you very much.

I have just tried to find the HashSet in its namespace using System.Collections; but it seems to only find HashTable. (I dont know if HashSet is part of the .Net Compact Framework?)

Quote:

---

Originally Posted by demise

I have just tried to find the HashSet in its namespace using System.Collections; but it seems to only find HashTable. (I dont know if HashSet is part of the .Net Compact Framework?)

---

Hi there.

The class is defined in System.Collections.Generic. I've not come across it before so I'll have to look into it. It appears to be new in .NET 3.5. It doesn't seem to be supported in the Compact Framework though. At least it's not stated on MSDN (HashSet of T).

Quote:

---

Originally Posted by nelo

I've not come across it before so I'll have to look into it.

---

it's very usefull if you have to do merge and difference etc. operations on collections.

Quote:

---

Originally Posted by nelo

It appears to be new in .NET 3.5. It doesn't seem to be supported in the Compact Framework though. At least it's not stated on MSDN (HashSet of T).

---

oops, I forgot that this is a mobile device with the compact version of the framework. then you have to use some other collection to store the selected picture boxes but the concept stays the same ;)

I have a slight problem.. (again :p).

I implemented an ArrayList to hold the picture boxes.

However.. when the form closes and reopen later on. I try to restore the picture boxes colour via:

Code:

---

foreach (PictureBox pbox in ptemp)
            {
                pbox.BackColor = Color.Red;
            }

---

ptemp being the ArrayList retrieved from another class.

I recieve the error message: Object Disposed Exception was unhandled.


Am I right to assume once the form closed, the ArrayList no longer points to existing picture boxes and therefore cannot be used? How can I go about this?

Thanks.

Quote:

---

Originally Posted by demise

Am I right to assume once the form closed, the ArrayList no longer points to existing picture boxes and therefore cannot be used?

---

that's right.

Quote:

---

Originally Posted by demise

How can I go about this?

---

you'll have to recognize them by names for example: PictureBox.Name and you'll have to store their names in the array instead of object references.,

Personally, I think the approach is incorrect (even with the improvement of the hash).

I believe you should separate the functionality of the picture box from the data. In other words, if the picture box toggles between red and black - wire that up separately from any data. Then expose a way to determine the state of the picture box.

I would probably create a user control that handles the picturebox toggling from red to black and expose an event (let's call it StateChanged) that gets sent when the user toggles the picturebox. In the event I would pass an Id that represents (an instance) of the control and also pass the toggle state (red or black).

Within the form I would create several custom controls and wire up the StateChange event in each of the controls to the same event handler. In the event handler I would change the state in the hash table based on the Id and state value.

This might sound like a bit of work, but in the end you'll get a cleaner implementation with a good separation of UI logic and data. The nice thing about this approach is that it lets you focus on just one aspect of the problem at a time rather than solving them both at once.

Quote:

---

Originally Posted by Arjay

Personally, I think the approach is incorrect (even with the improvement of the hash).

I believe you should separate the functionality of the picture box from the data. In other words, if the picture box toggles between red and black - wire that up separately from any data. Then expose a way to determine the state of the picture box.

I would probably create a user control that handles the picturebox toggling from red to black and expose an event (let's call it StateChanged) that gets sent when the user toggles the picturebox. In the event I would pass an Id that represents (an instance) of the control and also pass the toggle state (red or black).

Within the form I would create several custom controls and wire up the StateChange event in each of the controls to the same event handler. In the event handler I would change the state in the hash table based on the Id and state value.

This might sound like a bit of work, but in the end you'll get a cleaner implementation with a good separation of UI logic and data. The nice thing about this approach is that it lets you focus on just one aspect of the problem at a time rather than solving them both at once.

---

The logic here sounds like a very clean good idea. Sadly I don't think I fully understand how to implement it. I don't know what to do when you suggest to create a custom control which handles the picturebox toggle or how to expose the event.

Perhaps I do know how, but I don't understand the terminology. :/

Zip up a small sample project that toggles the red/black of the picture boxes and I'll modify it.

Quote:

---

Originally Posted by Arjay

Zip up a small sample project that toggles the red/black of the picture boxes and I'll modify it.

---

That is very kind Arjay.

I have uploaded it here: http://www.quickfilepost.com/downloa...f7d24b89a152c6

Thank you very much.

I can't open the Mobile 6 project type, if you want me to take a look at this, zip up a regular WinForms project.

Sorry about that.

I have created it as a windows form app here:
http://www.quickfilepost.com/downloa...98d93b39b7fbc7

Thank you :)

Demise, here you go. (btw, you can simply attach a zip file to a post directly by using the ManageAttachments button).

I've created a PictureBoxUC user control that simply toggles between Red/Black when the user clicks on the control. The user control also fires off a SelChange event that sends out Id and IsSelected event info.

I'm using a Guid Id as a representation of some entity (e.g. an industrial Machine). I set the Id for each PictureBoxUC control in the designer of Form1.

It is important to understand that the Id doesn't represent a PictureBoxUC user control instance (unlike your original code where you tried to store pictureBox control instances in an array).

Instead of tracking user controls, I've created a singleton class called a Model and it tracks the state info for each Id (i.e. each Machine). This is kind of a view model architecture and although it is very simple, because it only tracks a [machine] id, and whether it's been selected, the concept can be expanded to set a collection of data per machine.

Once I created the PictureBoxUC control, I dragged a couple of them onto the Form1 and set their [machine] Id's in the properties page and also wired up the SelChanged event. Because I'm passing in the Id with each SelChange event, I'm able to wire up each control to the same SelChanged event handler. This makes the code nice and tidy.

Code:

---


privatevoid  OnSelChanged( object sender, PictureBoxSelChangedEventArgs e )
{
  Model.Instance.SetState( e.Id, e.IsSelected );
}


---

You'll notice that within this handler I call Model.Instance.SetState( ). This calls the SetState method in the singleton Model class and stores the Id, and selection state into a dictionary.

In order to prove that the states are getting stored properly, I've added a View button which displays a MessageBox with the Id's and their selected states. This just loops through a States array that I've expose from the Dictionary in the Model. Normally I wouldn't expose a dictionary like this, but this is sample code.

Code:

---


privatevoid  _viewBtn_Click( object sender, EventArgs e )
{
  var sb = newStringBuilder( );

  foreach ( var kvp inModel.Instance.States )
  {
    sb.AppendFormat( "Id: {0}\tIsSelected: {1}\n", kvp.Key, kvp.Value );
  }
  MessageBox.Show( sb.ToString( ) );
}


---

Thank you very much Arjay. I really appreciate the time you have given me to help me. :)

I will implement this method for sure, thank you again!