Else If, Error Checking Done More Efficiently?

Question

What's a more efficient way to error check my UI without copy/pasting the same code over and over again?

I'm currently using repetitive else if () statements and I'm sure they're a more efficient way of doing it. I thought about using for loops but the different UI names makes it hard to implement into a for loop.

private void BoatSubmitButton_Click(object sender, EventArgs e)
{
    if (BoatNameTextBox.Text == "")
        {
        MessageBox.Show("Please Input Name", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
    else if (Catch1ComboBox.SelectedIndex != -1 && NumericAmount1.Value == 0)
        {
        MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
    else if (Catch2ComboBox.SelectedIndex != -1 && NumericAmount2.Value == 0 
        )
        {
        MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
    else if (Catch3ComboBox.SelectedIndex != -1 && NumericAmount3.Value == 0)
        {
        MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
    else if (Catch4ComboBox.SelectedIndex != -1 && NumericAmount4.Value == 0)
        {
        MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
}

An even more efficient way of error checking is using OR in the condition for example:

else if (Catch1ComboBox.SelectedIndex != -1 && NumericAmount1.Value == 0 ||
Catch2ComboBox.SelectedIndex != -1 && NumericAmount2.Value == 0 ||
Catch3ComboBox.SelectedIndex != -1 && NumericAmount3.Value == 0 ||
Catch4ComboBox.SelectedIndex != -1 && NumericAmount4.Value == 0)
{
    MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
}

Use a method or delegate of some type rather than copying and pasting the code. If the list of conditions is likely to change/grow, you could change them into a list of delegates that when evaluated to true, execute the method that displays the MessageBox — gabriel.hayes, Oct 23 '19 at 20:03
What is the original `if` for these `else if`s? And are there more `else if`s not shown? — elmer007, Oct 23 '19 at 20:07
wher is this code situated? in an event handler? `SelectedIndexChanged` ? or some ClickHandler? — Mong Zhu, Oct 23 '19 at 20:08
@Mong Zhu The statements are in a button_click event handler — Gentian Gashi, Oct 23 '19 at 20:12
https://stackoverflow.com/questions/11260681/get-value-from-multiple-textbox-elements is likely what you are looking for... also "more efficient" is really unclear explanation of what you trying to achieve making the question poor fit for SO. — Alexei Levenkov, Oct 23 '19 at 20:13
You could put the combination of a combobox and the corresponding numericUpDown into a groupbox, then put all those 4 group boxes into a single groupbox. Then you could iterate over the controls of the most outer groupbox and check it for every pair of combobox and numericUpDown. Alternatively to the loop you could also use `OfType` and Linq — Mong Zhu, Oct 23 '19 at 20:16
@elmer007 I've updated the post which includes the things you've asked — Gentian Gashi, Oct 23 '19 at 20:20
are you looking for a way to decrease the amount of code for such a case? do you intend to extend the amount of comboboxes and numericUpDowns? — Mong Zhu, Oct 23 '19 at 20:22
@MongZhu Yes that's right, I'm trying to decrease the amount of code I'm using — Gentian Gashi, Oct 23 '19 at 20:25

itsMasoud · Accepted Answer · 2019-10-23T20:44:25.197

2

I personally prefer to wrap condition in a method and then pass my values to it in only one if statement:

public static bool Check(ComboBox combo, NumericUpDown amount)
{
    if (combo.SelectedIndex != -1 && amount.Value == 0)
        return false;
    return true;
}

And then call Check method this way:

if (!Check(Catch1ComboBox, NumericAmount1) || !Check(Catch2ComboBox, NumericAmount2) ||
   !Check(Catch3ComboBox, NumericAmount3) || !Check(Catch4ComboBox, NumericAmount4))
   MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);

In more complicated problems it's better to use Rules design pattern.

edited Oct 23 '19 at 20:44

answered Oct 23 '19 at 20:23

itsMasoud

1,335
2
9
25

`NumericAmount amount)` NumericAmount is underlined with an error stating - `The type or namespace could not be found` Any ideas? – Gentian Gashi Oct 23 '19 at 20:39
What is the type of NumericAmount? – itsMasoud Oct 23 '19 at 20:40
It's a NumericUpDown box – Gentian Gashi Oct 23 '19 at 20:43
1

Change the `NumericAmount` in Check method to `NumericUpDown` as I did in answer. – itsMasoud Oct 23 '19 at 20:44

Olivier Jacot-Descombes · Answer 2 · 2019-10-23T20:39:43.217

With an array of ValueTuples (C# 7.0) you can make it like this:

if (BoatNameTextBox.Text == "") {
    MessageBox.Show("Please Input Name", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
} else {
    (ComboBox cbo, NumericUpDown nud)[] input = {
        (Catch1ComboBox, NumericAmount1),
        (Catch2ComboBox, NumericAmount2),
        (Catch3ComboBox, NumericAmount3),
        (Catch4ComboBox, NumericAmount4)
    };

    if (input.Any(x => x.cbo.SelectedIndex != -1 && x.nud.Value == 0)) {
        MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR",
            MessageBoxButtons.OK, MessageBoxIcon.Error);
    }
}

You could also make the array a class field and set it up in the constructor.

Named tuples ! That's a great idea. – Larry Oct 23 '19 at 20:38 — Larry, Oct 23 '19 at 20:38

score 0 · Answer 3 · answered Oct 23 '19 at 20:14

In general, you use else if if you need to do different things for each case. If you need to do the same thing, then I would combine all the cases into one, and just split them up with line breaks to make it easier to read:

else if ((Catch1ComboBox.SelectedIndex != -1 && NumericAmount1.Value == 0) ||
         (Catch2ComboBox.SelectedIndex != -1 && NumericAmount2.Value == 0) ||
         (Catch3ComboBox.SelectedIndex != -1 && NumericAmount3.Value == 0) ||
         (Catch4ComboBox.SelectedIndex != -1 && NumericAmount4.Value == 0)
{
    MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
}

Edit: Which, I now see you added to your question....

I wouldn't bother with loops or anything unless you had a lot more of these. If there really are only 4, then I would leave it be.

score 0 · Answer 4 · answered Oct 23 '19 at 20:27

EDit: sorry typo in signature

You could clean it up by adding an extension method

public static void isSelected(this ComboBox comboBox){
       return comboBox.SelectedIndex != -1;
} 

public static bool IsZero(this NumericAmount amt){
       return amt.Value == 0;
} //or w/e type NumericAmount actually is

THen in your if/else

if ((!Catch1ComboBox.isSelected() && NumericAmount1.IsZero()) ||
         (!Catch2ComboBox.isSelected() && NumericAmount2.IsZero()) ||
         (!Catch3ComboBox.isSelected()  && NumericAmount3.IsZero()) ||
         (!Catch4ComboBox.isSelected() && NumericAmount4.IsZero())
{
    MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
}

I'm pretty sure in winforms you can also select all 4 of those like html classes and run the extension method, that way you only have 1 validation call for each class. — Train, Oct 23 '19 at 20:33

Erik Philips · Answer 5 · 2019-10-24T17:03:51.590

0

When logic gets complicated, sometimes it better to flip the logic into their own evaluators (as functions). It provides a much easier way to read and maintain specific errors, and reduces the whole if, else repetition.

var List<Func<bool>> hasInputFishErrorConditions = new List<Func<bool>>()
{
  () => Catch1ComboBox.SelectedIndex != -1 && NumericAmount1.Value == 0,
  () => Catch2ComboBox.SelectedIndex != -1 && NumericAmount2.Value == 0,
  // etc
};

Then it's the code looks like:

var hasInputFishError = hasInputFishErrorConditions.Any();

if (hasInputFishError) {
 MessageBox.Show("Please Input Amount Of Fish Caught",
        "ERROR", 
        MessageBoxButtons.OK, 
        MessageBoxIcon.Error);
    };
} else {
  // no error condition
}

edited Oct 24 '19 at 17:03

answered Oct 23 '19 at 20:34

Erik Philips

53,428
11
128
150

1

I think you missed that the `SelectedIndex != -1` comparisons were using different combo boxes. – Gabriel Luci Oct 24 '19 at 16:27
@GabrielLuci I did totally miss that. Thank you. – Erik Philips Oct 24 '19 at 17:04

score 0 · Answer 6 · answered Oct 23 '19 at 20:37

The best is to have some validation framework, but I often use a loop when such a case arise:

var myCombos = new[] { Catch1ComboBox, Catch2ComboBox, Catch3ComboBox, Catch4ComboBox };
var myNumTextboxes = new [] { NumericAmount1, NumericAmount2, NumericAmount3, NumericAmount4 };

foreach(var x in myCombos.Zip(myNumTextboxes, (combo, numtextbox) => new { combo, numtextbox })) {
    if(x.combo.SelectedIndex != -1 && x.numtextbox.Value == 0) {
        MessageBox.Show("Please Input Amount Of Fish Caught", "ERROR", MessageBoxButtons.OK, MessageBoxIcon.Error);
        break;
    }
}

}

Else If, Error Checking Done More Efficiently?

6 Answers6