Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The problem I'm dealing with is the length of my code. I have lots of code like this:

if (comboBox1.SelectedIndex == 0)
{
    pictureBox1.Visible = true;
    pictureBox2.Visible = true;
    pictureBox3.Visible = true;
    pictureBox4.Visible = true;
}
if (comboBox1.SelectedIndex == 1)
{
    pictureBox1.Visible = false;
    pictureBox2.Visible = false;
    pictureBox3.Visible = false;
    pictureBox4.Visible = false;
}
if (comboBox1.SelectedIndex == 2)
{
    pictureBox1.Visible = false;
    pictureBox2.Visible = false;
    pictureBox3.Visible = false;
    pictureBox4.Visible = false;
}

...And I just realized, that I will have to add about 10 labels to each index as well. Can anyone tell me how to make this whole thing shorter? I thought about creating a class, that would contain those properties and then just use the class in if's.

share

migration rejected from stackoverflow.com Apr 24 at 6:24

This question came from our site for professional and enthusiast programmers. Votes, comments, and answers are locked due to the question being closed here, but it may be eligible for editing and reopening on the site where it originated.

closed as off-topic by Jamal Apr 24 at 6:24

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

    
do you have any other picture Boxes other than above in your form/container? if not you can try this.Controls.OfType<PictureBox>().ForEach(c => c.Visible = comboBox1.SelectedIndex == 0); –  Damith Jan 3 '14 at 17:43
    
You could at least convert to else if since the SelectedIndex check is mutually exclusive. –  swandog Jan 3 '14 at 17:48

6 Answers 6

up vote 38 down vote accepted

Instead of using a huge if statement, determine whether your controls should be visible or not in a single line:

bool visible = comboBox1.SelectedIndex == 0;

Then use this variable to modify your controls:

pictureBox1.Visible = visible;
pictureBox2.Visible = visible;
pictureBox3.Visible = visible;
pictureBox4.Visible = visible;

And you can shorten this even more by putting the controls you want to modify in a list:

var controls = new List<Control> {
    pictureBox1, 
    pictureBox2,
    pictureBox3,
    pictureBox4
};

Then iterating over that list like this:

bool visible = comboBox1.SelectedIndex == 0;
foreach (var c in controls) {
    c.Visible = visible;
}

Or like this:

bool visible = comboBox1.SelectedIndex == 0;
controls.ForEach(c => c.Visible = visible);

Which form loop you prefer is mostly a matter of style, but Eric Lippert has written a good commentary on this subject.

share
    
Yes. I personally like that. Of course, we don't know if there are any "selectedindex" values, where we could have a mix of visible true and false, so it may require 4 variables. var PicBox1Visible = (comboBox1.SelectedIndex == 0 || comboBox1.SelectedIndex == whatever || etc); var PicBox2Visible = (blah, blah) –  JustLooking Jan 3 '14 at 17:29
    
@JustLooking That's true. It might require more variables, or more conditions, and it's possible OP is modifying more than just .Visible on each control. –  p.s.w.g Jan 3 '14 at 17:30
    
I like this solution the best, I think. Even though you lose the "set one control, set all controls" capability of Sergey's, you keep the "collected approach" and manage everything in a fairly elegant way. –  Brian Warshaw Jan 6 '14 at 12:11

For this kind of scenario I typically use a "viewmodel" object that encapsulate the state, and then I use simple data-binding to control the visibility of associated controls.

For example, on this viewmodel class, you'd implement INotifyPropertyChanged, and have a ControlsVisible bool property, and an integer SelectedValue property:

public class ViewModel : INotifyPropertyChanged
{
    public bool ControlsVisible
    {
        get { return SelectedValue == 0; }
    }

    private int selectedValue;
    public bool SelectedValue
    {
        get { return selectedValue; }
        set
        {
            if(value != selectedValue)
            {
                selectedValue = value;
                OnNotifyPropertyChanged("SelectedValue");
            }
         }
    }

    public event PropertyChangedEventHandler PropertyChanged;

    protected void OnPropertyChanged(string propertyName)
    {
        if (this.PropertyChanged != null)
        {
            this.PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
        }
    }
}

I would then generate a "Project DataSource" (Project > Add New Data Source ...) and select that ViewModel class.

The next step is to put a BindingSource on your WinForm, and set its DataSource to the ViewModel class. Remember to instantiate an object of type ViewModel, and set it at runtime on the BindingSource.DataSource property. I typically do this in an OnLoad override.

Then, you data bind the Visible property on each PictureBox to the BindingSource - ControlsVisible property.

Finally, you data bind set the ComboBox's SelectedValue Binding to the BindingSource - SelectedValue property.

For this to work I'm assuming that your ComboBox's DataSource is set and that its SelectedValue property is set to return an int. You might have to adapt the actual type of the SelectedValue on the ViewModel.

While this might look like a lot of work at first sight, once you've implemented this pattern a few times in your code, it will become second nature and you won't have to worry about modifying your code in many places. You just need to modify the ControlVisible's condition and let the Windows Forms simple data-binding mechanism do its job.

Cheers

share

The answer provided already is a decent one, but if you don't want to nest them in some parent, try this:

pictureBox1.Visible = pictureBox2.Visible = pictureBox3.Visible = pictureBox4.Visible = (comboBox1.SelectedIndex == 0);
share

Given that you're checking if a combo box's index is a fixed integer value you could start by changing to a switch statement rather than a lot of if-statements. At the moment your program will check all the if conditions which is unnecessary if a combo box can only be one value at any given time.

As for the contents of each block of code inside the if-statements, assuming they are all the same as above, possibly consider consider creating a separate method that sets the visibility of those components:

switch(comboBox1.selectedIndex)
{
case 0:
setVisibility(true, true, true, true);
break;
}
...
//add new cases for each selectedIndex value

private void setVisibility(boolean visibility1, boolean visibility12, boolean visibility13, boolean visibility14)
{
  pictureBox1.Visible = visibility1;
  pictureBox2.Visible = visibility2;
  pictureBox3.Visible = visibility3;
  pictureBox4.Visible = visibility4;
}
share

Since comboBox1.SelectedIndex == 0 is the only case when you return true, this should do the trick (but only if this is really the only way when it should be true!!!):

pictureBox1.Visible = pictureBox2.Visible = pictureBox3.Visible = pictureBox4.Visible = (comboBox1.SelectedIndex == 0);
share

In more generic scenarios (where your state can be any permutation of true and false) I would use a matrix for the states. This offers a reasonable short code and is still understandable. Feel free to translate that code into your language:

Boolean[][] states = new Boolean[][] {
    {true,  true,  true,  true},
    {false, false, false, false},
    {false, false, false, false},
    {true,  false, true,  false} // new case to make the point of this solution.
};

assert isValid(comboBox1.SelectedIndex); // between 0 and states.length

Boolean[] state = states[comboBox1.selectedIndex];

pictureBox1.Visible = state[0];
pictureBox2.Visible = state[1];
pictureBox3.Visible = state[2];
pictureBox4.Visible = state[3];

Note that I added the new state 3 intentionally to make the point of this implementation.

share
1  
This will need some heavy documentation (row, column headers) to make the boolean matrix readable. It might still prove useful in some cases, though. –  Jan Dvorak Jan 4 '14 at 6:01

Not the answer you're looking for? Browse other questions tagged or ask your own question.