Looping through the values of two ComboBoxes by pressing a button

I have a C# application where I have implemented the MVP pattern. On one of my forms I have two ComboBoxes with values through which I loop with a button press. The first ComboBox contains different scenarios and the second one contains different (threat)actors. I show each scenario, and for each scenario I show each actor.

The button press event is triggered in my AnalysisScreenView class, to which my AnalysisScreenPresenter responds.

The code in the Presenter that loops through the values seems a little clunky to me. I hope someone could give me some improvements in terms of speed or style.

AnalysisScreenView - class where the button press is registered and the event is fired.

```
public partial class AnalysisScreenView : Form, IAnalysisScreenView
{
// Private members.
private AnalysisScreenPresenter _presenter;

// Public members.
public List Scenarios
{
get
{
var scenarios = new List();
foreach(var item in scenarioInput.Items)
{
scenarios.Add(item.ToString());
}
return scenarios;
}
set
{
foreach(var scen in value)
{
this.scenarioInput.Items.Add(scen);
}
}
}
public List ThreatActors
{
get
{
var actors = new List();
foreach (var item in actorInput.Items)
{
actors.Add(item.ToString());
}
return actors;
}
set
{
foreach(var actor in value)
{
this.actorInput.Items.Add(actor);
}
}
}
public string SelectedScenario
{
get
{
string scenarios = null;
if (scenarioInput.SelectedItem != null)
{
scenarios = this.scenarioInput.SelectedItem.ToString();
}
return scenarios;
}
s

public List Scenarios
{
    get
    {
        var scenarios = new List();
        foreach(var item in scenarioInput.Items)
        {
            scenarios.Add(item.ToString());
        }
        return scenarios;
    }
    set
    {
        foreach(var scen in value)
        {
            this.scenarioInput.Items.Add(scen);
        }
    }
}

This property (the other two similar as well) are very fragile. Everytime you use the setter you add new values to the ComboBoxes. I guess you want to call Clear before adding new items.

They can also be simplified because you can use the AddRange method instead of looping yourself.

scenarios.Items.Clear();
scenarios.Items.AddRange(value);

There is one more thing about the ComboBoxes that will signifficantly simplify the rest of your code.

Their items can be complex types and not only strings. This means you can add Scenario or ThreatActor as items and set the DisplayMember to something like Name or anything else you are showing now.

When you apply this to

public string SelectedScenario
{
    get
    {
        string scenarios = null;
        if (scenarioInput.SelectedItem != null)
        {
            scenarios = this.scenarioInput.SelectedItem.ToString();
        }
        return scenarios;
    }
    set { this.scenarioInput.SelectedItem = value; }
}

the getter will then become as simple as

public ThreatActor SelectedThreatActor
{
    get { return actorInput.SelectedItem; }
    set { this.actorInput.SelectedItem = value; }
}

Now you can make the other properties much simpler too for example Scenarios

public List Scenarios
{
    get { return scenarioInput.Items.Cast().ToList(); }
    set
    {
        scenarioInput.Items.Clear();
        scenarioInput.Items.AddRange(value);
    }
}

Consequently the SetNext event handler doesn't need to search for the actor or scenario anymore. I think this should work:

public void SetNext(object sender, EventArgs e)
{
    _view.SelectedThreatActor = _view.SelectedThreatActor ?? _view.ThreatActors.FirstOrDefault();      

    _view.SelectedScenario = _view.SelectedScenario ?? _view.Scenarios.FirstOrDefault();
}

public void SetNext(object sender, EventArgs e)

This event handler should be private and if you want to follow event handling conventions then its name should rather be view_SettingNext. By giving it a different name the readability suffers and in few weeks you'll need to look up how it's wired to know what it handles. Save your time by not using shortcuts and by giving things proper names from the very beginning. This was actually the only bad name in your code, the rest is fine.

StackExchange Code Review Q#151635, answer score: 3