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|improve this question

migrated from stackoverflow.com Jan 3 at 17:24

This question came from our site for professional and enthusiast programmers.

    
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 at 17:43
    
You could at least convert to else if since the SelectedIndex check is mutually exclusive. –  swandog Jan 3 at 17:48
add comment

8 Answers 8

up vote 37 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|improve this answer
    
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 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 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 at 12:11
add comment

I suggest you to place your controls on some user control or place them on existing container control (Panel, GroupBox, TableLayoutPanel, FlowLayoutPanel), and then just hide or show that control. You even can simplify conditional logic:

fooControl.Visible = (comboBox1.SelectedIndex == 0);
share|improve this answer
1  
I think the problem is creating a separate user control just for hosting these. It might be more appropriate to use a GroupBox. –  Brian Warshaw Jan 3 at 17:32
1  
@JustLooking which 8 possible configurations you are talking about? There is only two configurations - either all controls visible, or all controls hidden –  Sergey Berezovskiy Jan 3 at 17:35
1  
It's not duplicating anything. It's taking a group of controls that will share common visibility based on the state of another control and grouping them together. We can nullify any approach based on theoretical eventualities, but to solve his current problem, having the controls grouped is as valid as any other suggestion, because one GroupBox will take care of the problem. –  Brian Warshaw Jan 3 at 17:44
2  
@JustLooking I don't see any need in the original question or the solution to create multiple GroupBox instances or duplications of the PictureBox instances. He's currently got 4 PictureBox instances that need to be hidden. lazyb is saying (I hope) that you group the four instances in one GroupBox instance, and then set visibility on the GroupBox. Add or remove PictureBox instances in the future? The visibility code doesn't have to change. –  Brian Warshaw Jan 3 at 18:00
1  
@JustLooking I see your point. If selectedIndex == 8 means two pictureBoxes are visible and two aren't then I definitely would change solution. But not before I really needed (YAGNI). And in that case I will stop and think why those two pictureboxes handled differently. E.g. if we have form which shows weather forecast, and those two images are related to humidity, then, I would try to show that in my design (possibly new HumidityPanel will be introduced). I like to have things logically grouped, if I need to handle them as a whole. That would be grouping control in winforms or some div in web –  Sergey Berezovskiy Jan 3 at 18:08
show 18 more comments

How about:

void SetVisible(bool state)
{
  pictureBox1.Visible = state;
  pictureBox2.Visible = state;
  pictureBox3.Visible = state;
  pictureBox4.Visible = state;
}

if (comboBox1.SelectedIndex == 0) SetVisible(true);
if (comboBox1.SelectedIndex == 1) SetVisible(false);
if (comboBox1.SelectedIndex == 2) SetVisible(false);
share|improve this answer
    
+1, this was going to be my suggestion –  Cruncher Jan 3 at 19:12
9  
Why not just SetVisible(comboBox1.SelectedIndex == 0)? –  p.s.w.g Jan 3 at 19:20
2  
I think a switch would be better there. –  dyesdyes Jan 3 at 21:30
add comment

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|improve this answer
add comment

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|improve this answer
add comment

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|improve this answer
add comment

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|improve this answer
add comment

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|improve this answer
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 at 6:01
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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