Looping through the values of two ComboBoxes by pressing a button

Question

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<string> Scenarios
    {
        get
        {
            var scenarios = new List<string>();
            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<string> ThreatActors
    {
        get
        {
            var actors = new List<string>();
            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;
        }
        set { this.scenarioInput.SelectedItem = value; }
    }
    public string SelectedThreatActor
    {
        get
        {
            string actors = null;
            if (actorInput.SelectedItem != null)
            {
                actors = this.actorInput.SelectedItem.ToString();
            }
            return actors;
        }
        set { this.actorInput.SelectedItem = value; }
    }

    // Public events.
    public event EventHandler SettingNext;

    // Initialize zone screen. Create an instance of the presenter with a reference the view itsself.
    public AnalysisScreenView(IRiskAnalysisModel model)
    {
        InitializeComponent();
        _presenter = new AnalysisScreenPresenter(this, model);
        Show();
    }

    // Selects the next actor or scenario.
    private void nextButton_Click(object sender, EventArgs e)
    {
        SettingNext?.Invoke(this, EventArgs.Empty);
    }
}

AnalysisScreenPresenter - class where the next actor or scenario is set.

public class AnalysisScreenPresenter
{
    // Private members.
    private readonly IAnalysisScreenView _view;
    private IRiskAnalysisModel _model;

    // Initialize the analysis screen presenter with an IAnalysisScreenView and a new risk analysis assessments model. Subsribe to necessary events.
    public AnalysisScreenPresenter(IAnalysisScreenView view, IRiskAnalysisModel model)
    {
        _view = view;
        _model = model;
        _view.SettingNext += SetNext;
    }

    // Sets the next actor or scenario.
    public void SetNext(object sender, EventArgs e)
    {
        var actors = _view.ThreatActors;
        int actorIndex = actors.IndexOf(_view.SelectedThreatActor);

        if(actorIndex >= 0 && actorIndex < actors.Count - 1)
        {
            _view.SelectedThreatActor = actors[actorIndex + 1];
            return;
        }

        _view.SelectedThreatActor = actors[0];

        var scenarios = _view.Scenarios;
        int scenarioIndex = scenarios.IndexOf(_view.SelectedScenario);

        if (scenarioIndex >= 0 && scenarioIndex < scenarios.Count - 1)
        {
            _view.SelectedScenario = scenarios[scenarioIndex + 1];
            return;
        }

        _view.SelectedScenario = scenarios[0];
    }
}

Answer 1

public List<string> Scenarios
{
    get
    {
        var scenarios = new List<string>();
        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<Scenario> Scenarios
{
    get { return scenarioInput.Items.Cast<Scenario>().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.