Avoiding too many if else, else if statement for multiple(9) conditions
I have got 9 condition for my if else statement , I was thinking is there any other alternative solution to make the code clean and short. All the 9 condition perform different calculations for asp.net mvc view . I have included the code and an image which shows the view of some conditions, hope it make sense , I was looking for any better and robust solution for this scenario.. thanks
//Condition 1
//when no selection is made for the 2 dropdownlist (garages & helmets)
if (formvalues["helmets"] == "" && formvalues["garages"] == "")
{
ViewBag.hideHelmet = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedHelmets = 1;//
ViewBag.SelectedGarages = 1;//
ViewBag.TotalHelmets = 1;
ViewBag.TotalGarages = 1;
ViewBag.TotalAmount = 1;
ViewBag.TotalAmount = trackEventCost.UnitCost;
}
//Condition 2
//When garages are selected from dropdown & helmets are not selected
else if ((formvalues["helmets"] == "") && (Convert.ToInt32(formvalues["garages"]) > 0))
{
ViewBag.hideHelmet = 0;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedHelmets = 1;
ViewBag.SelectedGarages = Convert.ToInt32(formvalues["garages"]);
ViewBag.TotalHelmets = 1;
ViewBag.TotalGarages = Convert.ToInt32(formvalues["garages"]) * getGarages.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalGarages + trackEventCost.UnitCost;
}
//Condition 3
//When helmets are selected from dropdown & garages are not selected
else if ((formvalues["garages"] == "") && (Convert.ToInt32(formvalues["helmets"]) > 0))
{
ViewBag.hideHelmet = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 0;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedGarages = 1;
ViewBag.SelectedHelmets = Convert.ToInt32(formvalues["helmets"]);
ViewBag.TotalGarages = 1;
ViewBag.TotalHelmets = Convert.ToInt32(formvalues["helmets"]) * getGarages.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalHelmets + trackEventCost.UnitCost;
}
//Condition 4
//When garages are not selected from dropdown & helmets dropdownlist is hidden on the view due to unavailablity of helmets for that event
else if (formvalues["garages"] == "" && (formvalues["helmets"] == null))
{
ViewBag.hideHelmet = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.TotalAmount = trackEventCost.UnitCost;
}
//Condition 5
//When helmets are not selected from dropdown & garages dropdownlist is hidden on the view due to unavailablity of garages for that event
else if ((formvalues["garages"] == null) && (formvalues["helmets"] == ""))
{
ViewBag.hideHelmet = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedHelmets = 1;
ViewBag.TotalAmount = trackEventCost.UnitCost;
}
//Condition 6
//When both helmets and garages dropdown list is not displayed on the view as they are not present in the database
else if (formvalues["helmets"] == null && formvalues["garages"] == null)
{
ViewBag.SelectedHelmets = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.TotalHelmets = 1;
ViewBag.TotalGarages = 1;
ViewBag.hideHelmet = 1;
ViewBag.hideGarages = 1;
ViewBag.TotalAmount = trackEventCost.UnitCost;
}
//Condition 7
//When garages are selected from dropdown & helmets dropdownlist is hidden on the view due to unavailablity of helmets for that event
else if (formvalues["helmets"] == null && Convert.ToInt32(formvalues["garages"]) > 0)
{
ViewBag.hideHelmet = 0; //for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1; //for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedGarages = Convert.ToInt32(formvalues["garages"]);
ViewBag.TotalGarages = Convert.ToInt32(formvalues["garages"]) * getGarages.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalGarages + trackEventCost.UnitCost;
}
//Condition 8
//When helmets are selected from dropdown & garages dropdownlist is hidden on the view due to unavailablity of garages for that event
else if (Convert.ToInt32(formvalues["helmets"]) > 0 && formvalues["garages"] == null)
{
ViewBag.hideHelmet = 1; //for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 0; //for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.TotalHelmets = Convert.ToInt32(formvalues["helmets"]) * getHelmets.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalHelmets + trackEventCost.UnitCost;
}
//Condition 9
//When garages and helmet both dropdown are selected
else
{
ViewBag.hideHelmet = 0;//for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 0;//for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedHelmets = Convert.ToInt32(formvalues["helmets"]);
ViewBag.SelectedGarages = Convert.ToInt32(formvalues["garages"]);
ViewBag.TotalHelmets = Convert.ToInt32(formvalues["helmets"]) * getHelmets.UnitCost;
View开发者_JS百科Bag.TotalGarages = Convert.ToInt32(formvalues["garages"]) * getGarages.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalHelmets + ViewBag.TotalGarages + trackEventCost.UnitCost;
}
Certainly 1 thing you could do is improve how you write your conditionals. I think you might be better served by extracting your conditionals into methods to describe what the check is actually doing instead of adding comments everywhere.
First, move the extraction of the form value to the beginning of the method, e.g:
int? helmets = formvalues["helmets"] == null ?
null : Convert.ToInt32(formvalues["helmets"];
int? garages = formvalues["garages"] == null ?
null : Convert.ToInt32(formvalues["garages"];
Then you can probably very easily set the properties without any ifs/else ifs, e.g:
ViewBag.hideHelmet = helmets;
// or
someOtherProperty = helmets == null ? ... : ...
Update (regarding your comment/question):
- The
?:
operator in the statementx = condition ? value1 : value2
is called conditional operator. It returnsvalue1
ifcondition
is true, andvalue2
otherwise. int?
is a nullable integer, which can either have an integer value or be null.
Why don't you focus on the assignment targets, instead of the conditions?
The condidition seems to be pretty simple and repeated. One sample:
ViewBag.hideHelmet = formvalues["helmets"] == ""? 1 : 0;
This doesn't need any branching logic.
You could abstract your ViewBag to a function that sets all of your data. This will clean things up a bit., e.g.
if(...) {
SetUpViewBag(...); }
else {
SetUpViewBag(...); }
...
private SetUpViewBag(...)
{
ViewBag.SelectedHelmets = prop1;
ViewBag.SelectedGarages = prop2;
ViewBag.TotalHelmets = prop3;
ViewBag.TotalGarages = prop4;
ViewBag.hideHelmet = prop5;
ViewBag.hideGarages = prop6;
ViewBag.TotalAmount = prop7;
}
You could move
ViewBag.hideHelmet = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedHelmets = 1;//
ViewBag.SelectedGarages = 1;//
ViewBag.TotalHelmets = 1;
ViewBag.TotalGarages = 1;
ViewBag.TotalAmount = 1;
ViewBag.TotalAmount = trackEventCost.UnitCost;
To a method with certain parameter as optional
because you dont set all the values in all cases. Secondly you could use ternary operator. You could use ternary operators like.
int r = 1;
long l = r == 1 ? 1 : (r==2 ? 2 : (r==3?3:(r==4 ? 4 : 5)));
Take a look at the Strategy pattern, for your case you have to use method per strategy rather than class per strategy.
void ApplyStrategy(int? helmets, int? garages)
{
// depends on values of helmets and garages execute appropriate strategy
}
void ApplyNoHelmetsNoGaragesStrategy()
void ApplyNoGaragesStrategy()
void ApplyNoHelmetsStrategy()
// ...
I would use the Factory Pattern to generate your ViewBag
object based on the conditions. Have the factory generate the desired ViewBag
and assign your object to that.
Here is an example in a console example (just for quick demonstration):
class MainApp
{
/// <summary>
/// Entry point into console application.
/// </summary>
static void Main()
{
ViewBagCreator creator = new ConcreteCreatorForViewBagObject();
string garages_form_value = ""; // formvalues["garages"];
string helmets_form_value = ""; // formvalues["helments"];
// create viewpage
ViewBagObject view_bag = creator.FactoryMethod(helmets_form_value,
garages_form_value, 10, 100, 23);
Console.WriteLine("Created {0}",
view_bag.GetType().Name);
// Assign your viewbag object here
// ViewBag = view_bag;
// Wait for user
Console.ReadKey();
}
}
/// <summary>
/// The 'ViewBagObject' abstract class
/// </summary>
abstract class ViewBagObject
{
public int hideHelmet;
public int hideGarages;
public int SelectedHelmets;
public int SelectedGarages;
public int TotalHelmets;
public int TotalGarages;
public int TotalAmount;
}
/// <summary>
/// A 'ViewBagNoSelection' class
/// </summary>
class ViewBagNoSelection : ViewBagObject
{
public ViewBagNoSelection(int trackEventUnitCost)
{
hideHelmet = 1;
hideGarages = 1;
SelectedHelmets = 1;
SelectedGarages = 1;
TotalHelmets = 1;
TotalGarages = 1;
TotalAmount = trackEventUnitCost;
}
}
/// <summary>
/// A 'ViewBagGaragesNoHelments' class
/// </summary>
class ViewBagGaragesNoHelments : ViewBagObject
{
public ViewBagGaragesNoHelments(int garagesValue, int garagesUnitCost, int trackEventUnitCost)
{
hideHelmet = 0;
hideGarages = 1;
SelectedHelmets = 1;
SelectedGarages = garagesValue;
TotalHelmets = 1;
TotalGarages = garagesValue * garagesUnitCost;
TotalAmount = TotalGarages * trackEventUnitCost;
}
}
/// <summary>
/// A 'ViewBagHelmentsNoGarages' class
/// </summary>
class ViewBagHelmentsNoGarages : ViewBagObject
{
public ViewBagHelmentsNoGarages(int helmetsValue, int helmentsUnitCost, int trackEventUnitCost)
{
hideHelmet = 0;
hideGarages = 1;
SelectedHelmets = helmetsValue;
SelectedGarages = 1;
TotalHelmets = helmetsValue * helmentsUnitCost;
TotalGarages = 1;
TotalAmount = TotalHelmets * trackEventUnitCost;
}
}
/// <summary>
/// The 'ViewBagCreator' abstract class
/// </summary>
abstract class ViewBagCreator
{
public abstract ViewBagObject FactoryMethod(
string helmetsFormValue, string garagesFormValue,
int helmetsUnitCost, int garagesUnitCost, int trackEventUnitCost);
}
/// <summary>
/// A 'ConcreteCreator' class
/// </summary>
class ConcreteCreatorForViewBagObject : ViewBagCreator
{
public override ViewBagObject FactoryMethod(
string helmetsFormValue, string garagesFormValue,
int helmetsUnitCost, int garagesUnitCost, int trackEventUnitCost)
{
bool helmets_value_is_null = (helmetsFormValue == null);
bool helmets_value_is_empty = (helmetsFormValue == "");
bool garages_value_is_null = (garagesFormValue == null);
bool garages_value_is_empty = (garagesFormValue == "");
int helmets = 0;
int garages = 0;
int.TryParse(garagesFormValue, out garages);
int.TryParse(helmetsFormValue, out helmets);
bool garages_greater_than_zero = garages > 0;
bool helmets_greater_than_zero = helmets > 0;
if (helmets_value_is_empty && garages_value_is_empty)
{
return new ViewBagNoSelection(trackEventUnitCost);
}
else if (helmets_value_is_empty && garages_greater_than_zero)
{
return new ViewBagGaragesNoHelments(garages, garagesUnitCost, trackEventUnitCost);
}
else if (garages_value_is_empty && helmets_greater_than_zero)
{
return new ViewBagHelmentsNoGarages(helmets, helmetsUnitCost, trackEventUnitCost);
}
//...
return null;
}
}
Some great suggestions. I like to handle these situations as follows. I've refactored your code to remove the conditionals entirely. Instead I've encapsulated the code for each condition into a method and created a hashtable with a key of type int
and a value of type Action
. I've derived a unique value for each helmet/garage combination which is then used as the key for the corresponding method.
I've included dummy implementations for the bits of code not in your post so that I could compile and run my refactored example. Hope that helps.
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
namespace StackOverflow
{
public enum HelmetState { Null = 1, Empty = 2, HasValue = 3 }
public enum GarageState { Null = 4, Empty = 5, HasValue = 6 }
public class ReducingConditionals
{
private TrackEventCost trackEventCost = new TrackEventCost();
private GetHelmets getHelmets = new GetHelmets();
private GetGarages getGarages = new GetGarages();
private IDictionary<int, Action> ExecuteCondition;
//the formvalues collection
private Hashtable _formvalues;
public Hashtable formvalues{ get { return _formvalues ?? (_formvalues = new Hashtable()); } }
//set up dictionary of Actions
public ReducingConditionals()
{
ExecuteCondition = new Dictionary<int, Action>();
ExecuteCondition[Key(HelmetState.Null, GarageState.Null)] = HelmetsNullGaragesNull;
ExecuteCondition[Key(HelmetState.Null, GarageState.Empty)] = HelmetsNullGaragesEmpty;
ExecuteCondition[Key(HelmetState.Null, GarageState.HasValue)] = HelmetsNullGaragesHasValue;
ExecuteCondition[Key(HelmetState.Empty, GarageState.Null)] = HelmetsEmptyGaragesNull;
ExecuteCondition[Key(HelmetState.Empty, GarageState.Empty)] = HelmetsEmptyGaragesEmpty;
ExecuteCondition[Key(HelmetState.Empty, GarageState.Empty)] = HelmetsEmptyGaragesHasValue;
ExecuteCondition[Key(HelmetState.HasValue, GarageState.Empty)] = HelmetsHasValueGaragesNull;
ExecuteCondition[Key(HelmetState.HasValue, GarageState.Null)] = HelmetsHasValueGaragesEmpty;
ExecuteCondition[Key(HelmetState.HasValue, GarageState.HasValue)] = AnyOtherCondition;
}
//gets a unique value for each HelmetState/GarageState combination to be used as a key to the dictionary
private int Key(HelmetState helmetState, GarageState garageState)
{
return (int)helmetState + (int)garageState;
}
//Execute the appropriate method - n.b. no if statements in sight!
public void DealWithConditions()
{
HelmetState helmetState = GetHelmetState(formvalues["helmets"]);
GarageState garageState = GetGarageState(formvalues["garages"]);
ExecuteCondition[Key(helmetState, garageState)]();
}
//assign helmet state enum
private HelmetState GetHelmetState(object helmetValue)
{
if (helmetValue == null) return HelmetState.Null;
if (helmetValue.ToString() == "") return HelmetState.Empty;
if (Convert.ToInt32(helmetValue) > 0) return HelmetState.HasValue;
throw new InvalidDataException("Unexpected parameter value");
}
//assign garage state enum
private GarageState GetGarageState(object garageValue)
{
if (garageValue == null) return GarageState.Null;
if (garageValue.ToString() == "") return GarageState.Empty;
if (Convert.ToInt32(garageValue) > 0) return GarageState.HasValue;
throw new InvalidDataException("Unexpected parameter value");
}
#region encapsulate conditions in methods
private void AnyOtherCondition()
{
ViewBag.hideHelmet = 0;//for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 0;//for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedHelmets = Convert.ToInt32(formvalues["helmets"]);
ViewBag.SelectedGarages = Convert.ToInt32(formvalues["garages"]);
ViewBag.TotalHelmets = Convert.ToInt32(formvalues["helmets"]) * getHelmets.UnitCost;
ViewBag.TotalGarages = Convert.ToInt32(formvalues["garages"]) * getGarages.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalHelmets + ViewBag.TotalGarages + trackEventCost.UnitCost;
}
private void HelmetsHasValueGaragesNull()
{
ViewBag.hideHelmet = 1; //for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 0; //for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.TotalHelmets = Convert.ToInt32(formvalues["helmets"]) * getHelmets.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalHelmets + trackEventCost.UnitCost;
}
private void HelmetsNullGaragesHasValue()
{
ViewBag.hideHelmet = 0; //for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1; //for jquery , This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedGarages = Convert.ToInt32(formvalues["garages"]);
ViewBag.TotalGarages = Convert.ToInt32(formvalues["garages"]) * getGarages.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalGarages + trackEventCost.UnitCost;
}
private void HelmetsNullGaragesNull()
{
ViewBag.SelectedHelmets = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.TotalHelmets = 1;
ViewBag.TotalGarages = 1;
ViewBag.hideHelmet = 1;
ViewBag.hideGarages = 1;
ViewBag.TotalAmount = trackEventCost.UnitCost;
}
private void HelmetsEmptyGaragesNull()
{
ViewBag.hideHelmet = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedHelmets = 1;
ViewBag.TotalAmount = trackEventCost.UnitCost;
}
private void HelmetsNullGaragesEmpty()
{
ViewBag.hideHelmet = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.TotalAmount = trackEventCost.UnitCost;
}
private void HelmetsHasValueGaragesEmpty()
{
ViewBag.hideHelmet = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 0;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedGarages = 1;
ViewBag.SelectedHelmets = Convert.ToInt32(formvalues["helmets"]);
ViewBag.TotalGarages = 1;
ViewBag.TotalHelmets = Convert.ToInt32(formvalues["helmets"]) * getGarages.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalHelmets + trackEventCost.UnitCost;
}
private void HelmetsEmptyGaragesHasValue()
{
ViewBag.hideHelmet = 0;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedHelmets = 1;
ViewBag.SelectedGarages = Convert.ToInt32(formvalues["garages"]);
ViewBag.TotalHelmets = 1;
ViewBag.TotalGarages = Convert.ToInt32(formvalues["garages"]) * getGarages.UnitCost;
ViewBag.TotalAmount = ViewBag.TotalGarages + trackEventCost.UnitCost;
}
private void HelmetsEmptyGaragesEmpty()
{
ViewBag.hideHelmet = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.hideGarages = 1;//for jquery, This value is passed to jquery script which then decides which field to hide/show
ViewBag.SelectedHelmets = 1;//
ViewBag.SelectedGarages = 1;//
ViewBag.TotalHelmets = 1;
ViewBag.TotalGarages = 1;
ViewBag.TotalAmount = 1;
ViewBag.TotalAmount = trackEventCost.UnitCost;
}
#endregion
}
#region dummy class class implementations
public class ViewBag
{
public static int TotalAmount;
public static int hideHelmet { get; set; }
public static int hideGarages { get; set; }
public static int SelectedHelmets { get; set; }
public static int SelectedGarages { get; set; }
public static int TotalGarages { get; set; }
public static int TotalHelmets { get; set; }
}
internal class GetGarages { public int UnitCost; }
internal class GetHelmets { public int UnitCost; }
internal class TrackEventCost{ public int UnitCost;}
#endregion
}
What about building a condition class
public class Condition
{
public Func<bool> Match { get; set; }
public Action Execute { get; set; }
}
and bulid a list
var conditions = new List<Condition>
{
new Condition
{
Match = () => formvalues["helmets"] == "" && formvalues["garages"] == "",
Action = () =>
{
ViewBag.hideHelmet = 1;
ViewBag.hideGarages = 1;
ViewBag.SelectedHelmets = 1;
...
}
},
new Condition
{
...
}
};
then linq:
conditions.Where(c => c.Match()).ToList().ForEach( c => c.Execute());
精彩评论