1
\$\begingroup\$

I have these if-else statements that I am not able to refactor. I have used them for doing validations on server-side using asp.net.

Would anyone please suggest ways to reduce these statements? Here, validation fields and validation types are enum lists.

else if (CheckNextItem(ddlBr1Type.SelectedValue.ToString(), ddlBr2Type.SelectedValue.ToString(), ValidationFields.FO.ToString(), ValidationTypes.P.ToString()))
{
   BrkTypeValidator2.ErrorMessage = "'TOP0618 -Invalid combination of Bracket IDs'";
   args.IsValid = false;
}
else if (CheckNextItem(ddlBr1Type.SelectedValue.ToString(), ddlBr2Type.SelectedValue.ToString(), ValidationFields.FW.ToString(), ValidationTypes.P.ToString()))
{
   BrkTypeValidator2.ErrorMessage = "'TOP0618 -Invalid combination of Bracket IDs'";
   args.IsValid = false;
}
else if (CheckNextItem(ddlBr1Type.SelectedValue.ToString(), ddlBr2Type.SelectedValue.ToString(), ValidationFields.UF.ToString(), ValidationTypes.P.ToString()))
{
     BrkTypeValidator2.ErrorMessage = "'TOP0618 -Invalid combination of Bracket IDs'";
     args.IsValid = false;
}
else if (CheckNextItem(ddlBr1Type.SelectedValue.ToString(), ddlBr2Type.SelectedValue.ToString(), ValidationTypes.O.ToString(), ValidationTypes.P.ToString()))
{
      BrkTypeValidator2.ErrorMessage = "'TOP0618 -Invalid combination of Bracket IDs'";
      args.IsValid = false;
}
else if (CheckNextItem(ddlBr1Type.SelectedValue.ToString(), ddlBr2Type.SelectedValue.ToString(), ValidationTypes.W.ToString(), ValidationTypes.P.ToString()))
{
      BrkTypeValidator2.ErrorMessage = "'TOP0618 -Invalid combination of Bracket IDs'";
      args.IsValid = false;

}
else if (CheckNextItem(ddlBr1Type.SelectedValue.ToString(), ddlBr2Type.SelectedValue.ToString(), ValidationTypes.P.ToString(), ValidationTypes.C.ToString()))
{
      BrkTypeValidator2.ErrorMessage = "'TOP0618 -Invalid combination of Bracket IDs'";
      args.IsValid = false;

}
else if (CheckNextItem(ddlBr1Type.SelectedValue.ToString(), ddlBr2Type.SelectedValue.ToString(), ValidationTypes.C.ToString(), ValidationTypes.U.ToString()))
{
      BrkTypeValidator2.ErrorMessage = "'TOP0618 -Invalid combination of Bracket IDs'";
      args.IsValid = false;

}

This is method for Checknextitem:

 public static bool CheckNextItem(string Compareitem1, string comnpareitem2, string items1, string items2)
  {

        var listContains = Compareitem1 == items1 && comnpareitem2 != items2;
        return listContains;

  }
\$\endgroup\$
  • \$\begingroup\$ Am I correct in assuming that each bracket 1 ID has only one valid bracket 2 ID? So, for instance, when ID 1 is "FO", then ID 2 must be "P"? \$\endgroup\$ – Steven Doggart Jul 22 '13 at 15:52
  • \$\begingroup\$ yes upto Id1 has value "P" ...... \$\endgroup\$ – Enigma State Jul 22 '13 at 15:55
2
\$\begingroup\$

If each ID1 can only have one valid corresponding ID2, then I would recommend using a dictionary object to store the valid combinations, like this:

Dictionary<string, string> validIds = new Dictionary<string,string>();
validIds[ValidationFields.FO.ToString()] = ValidationTypes.P.ToString();
validIds[ValidationFields.FW.ToString()] = ValidationTypes.P.ToString();
validIds[ValidationFields.UF.ToString()] = ValidationTypes.P.ToString();
// ...

Then, you can check to see if the currently selected values are valid by simply comparing them to the valid combinations in the dictionary, like this:

if (validIds[ddlBr1Type.SelectedValue.ToString()] != ddlBr2Type.SelectedValue.ToString())
{
    BrkTypeValidator2.ErrorMessage = "'TOP0618 -Invalid combination of Bracket IDs'";
    args.IsValid = false;
}

By doing it that way, you only need the single if statement. Another side benefit is that you can now store the list of valid combinations somewhere else, outside of your code, such as in a configuration file or a database. Then you could load the data from that data source into the dictionary at run-time.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ would you pls tell me,where i need to put that dictionary \$\endgroup\$ – Enigma State Jul 22 '13 at 16:04
  • \$\begingroup\$ I really can't, without knowing much more about the broader context of your project. In general terms, it would be best to create it and populate it once, when the application starts, but even that suggestion would depend on the type of application and how often the valid combinations may change, if ever. Even though it's not terribly efficient, it should work if you just created and populated it in the same method that was doing the if statement. \$\endgroup\$ – Steven Doggart Jul 22 '13 at 16:11
  • \$\begingroup\$ +1 for taking the code right out of my mouth. I use a Dictionary of <Condition,Action> but it's the same result \$\endgroup\$ – Gayot Fow Jul 22 '13 at 18:41
1
\$\begingroup\$

Alternatively, if you have only a relatively small number of valid values, you can use tuples and LINQ to perform the comparison.

You would initialize your collection of tuples with the valid values as follows:

_validValues = new []
                  {
                     new Tuple<string,string> ("FO", "P"),
                     new Tuple<string,string> ("FW", "P"),
                     new Tuple<string,string> ("UF", "P"),
                     new Tuple<string,string> ("O", "P"),
                     new Tuple<string,string> ("W", "P"),
                     new Tuple<string,string> ("P", "C"),
                     new Tuple<string,string> ("C", "U"),
                  };

Given how they were used, I made the (perhaps incorrect) assumption that your ValidationTypes and ValidationFields types are enums. If this is incorrect, feel free to substitute the _.ToString values back in.

The validation method could go as follows:

var currentValue = new Tuple<string,string> (ddlBr1Type.SelectedValue.ToString(), ddlBr2Type.SelectedValue.ToString());

args.IsValid = _validValues.Contains (currentvalue);
BrkTypeValidator2.ErrorMessage = args.IsValid ? null : ERROR;

And, of course, I would use a constant (or better yet, a localized string) for your error message.

private const string ERROR = "'TOP0618 -Invalid combination of Bracket IDs'";

The nice thing about a collection of tuples over a dictionary is that:

  • it allows for mutiple valid ddlBr2Type values for a given ddlBr1Type value without getting messy (a dictionary forces you to change-over to IDictionary<TKey,IEnumerable<TValue>>, whereas you just add more tuple values)
  • the code reads a bit clearer, as you define the valid set of values and ask if your current combination is contained within that set
|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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