Tree refactoring If else

Question

I have an if else tree that will grow as I add additional elements to support it, and I'm looking for a better way to write it for support. I start with this code.

private void ControlSelect() { if (PostingType == PostingTypes.Loads && !IsMultiPost) { singleLoadControl.Visible = true; singleTruckControl.Visible = false; multiTruckControl.Visible = false; multiLoadControl.Visible = false; } else if (PostingType == PostingTypes.Trucks && !IsMultiPost) { singleLoadControl.Visible = false; singleTruckControl.Visible = true; multiTruckControl.Visible = false; multiLoadControl.Visible = false; } else if (PostingType == PostingTypes.Loads && IsMultiPost) { singleLoadControl.Visible = false; singleTruckControl.Visible = false; multiTruckControl.Visible = false; multiLoadControl.Visible = true; } else if (PostingType == PostingTypes.Trucks && IsMultiPost) { singleLoadControl.Visible = false; singleTruckControl.Visible = false; multiTruckControl.Visible = true; multiLoadControl.Visible = false; } } 

and think about re-factoring it to something like this

 private void ControlSelect() { List<UserControl> controlList = GetControlList(); string visableControl = singleLoadControl.ID; if (PostingType == PostingTypes.Loads && !IsMultiPost) { visableControl = singleLoadControl.ID; } else if (PostingType == PostingTypes.Trucks && !IsMultiPost) { visableControl = singleTruckControl.ID; } else if (PostingType == PostingTypes.Loads && IsMultiPost) { visableControl = multiLoadControl.ID; } else if (PostingType == PostingTypes.Trucks && IsMultiPost) { visableControl = multiTruckControl.ID; } foreach (UserControl userControl in controlList) { userControl.Visible = (userControl.ID == visableControl); } } private List<UserControl> GetControlList() { List<UserControl> controlList = new List<UserControl> { singleLoadControl, multiTruckControl, singleTruckControl, multiLoadControl }; return controlList; } 

I take a performance hit, but I can control all my controls - this is one place

my other thought was to make each selected control its own method, something like this

 private void SetSingleLoadControlAsSelected() { singleLoadControl.Visible = true; singleTruckControl.Visible = false; multiTruckControl.Visible = false; multiLoadControl.Visible = false; } 

I do not take a hit in performance, but I support controls in several places

I am inclined to option one only because I like its support aspect.

Answer 1

What about

 singleLoadControl.Visible = PostingType == PostingTypes.Loads && !IsMultiPost; singleTruckControl.Visible = PostingType == PostingTypes.Trucks && !IsMultiPost; multiTruckControl.Visible = PostingType == PostingTypes.Loads && IsMultiPost; multiLoadControl.Visible = PostingType == PostingTypes.Trucks && IsMultiPost; 

if you want several visible controls (or add more enumerated values), decorate the enumeration with the [Flags] attribute as follows:

 [Flags] public enum PostTyp { None=0, IsMultiPost = 1, Loads = 2, Trucks = 4 } 

and change the code as follows:

 singleLoadControl.Visible = ((PostingType & (PostTyp.Loads | ~PostTyp.MultiCast)) == PostingType ); singleTruckControl.Visible = ((PostingType & (PostTyp.Trucks | ~PostTyp.MultiCast)) == PostingType ); multiTruckControl.Visible = ((PostingType & (PostTyp.Loads | PostTyp.MultiCast)) == PostingType ); multiLoadControl.Visible = ((PostingType & (PostTyp.Trucks | PostTyp.MultiCast)) == PostingType ); 

Answer 2

As you seem to be using an enumeration, I would recommend a switch with a standard case to handle unknown values. I believe this approach makes intentions clearer than performing all the checks in the assignment.

 switch (PostingType) { case PostingTypes.Loads: singleLoadControl.Visible = !IsMultiPost; multiTruckControl.Visible = IsMultiPost; singleTruckControl.Visible = false; multiTruckLoadControl.Visible = false; break; case PostingTypes.Trucks: singleLoadControl.Visible = false; multiTruckControl.Visible = false; singleTruckControl.Visible = !IsMultiPost; multiLoadControl.Visible = IsMultiPost; break; default: throw InvalidOperationException("Unknown enumeration value."); } 

Answer 3

How about this:

  singleLoadControl.Visible = false; singleTruckControl.Visible = false; multiTruckControl.Visible = false; multiLoadControl.Visible = false; if (PostingType == PostingTypes.Loads && !IsMultiPost) { singleLoadControl.Visible = true; } else if (PostingType == PostingTypes.Trucks && !IsMultiPost) { singleTruckControl.Visible = true; } else if (PostingType == PostingTypes.Loads && IsMultiPost) { multiLoadControl.Visible = true; } else if (PostingType == PostingTypes.Trucks && IsMultiPost) { multiTruckControl.Visible = true; } 

Answer 4

If that were reasonable (for example, if it is already domain-specific custom controls), you can encapsulate the logic inside the controls themselves ( Replace conditional with polymorphism ). Perhaps create an interface like this:

 public interface IPostingControl { void SetVisibility(PostingType postingType, bool isMultiPost); } 

Then each control will be responsible for its visibility rules:

 public class SingleLoadControl: UserControl, IPostingControl { // ... rest of the implementation public void SetVisibility(PostingType postingType, bool isMultiPost) { this.Visible = postingType == PostingType.Load && !isMultiPost); } } 

Finally, on your page, just go through your IPostingControls and call SetVisibility(postingType, isMultiPost) .

Answer 5

If you expect to add many different parameters, you can create a multidimensional array of objects

 Arr[0][0] = singleLoadControl Arr[0][1] = singleTruckControl Arr[1][0] = multiLoadControl Arr[1][1] = multiTruckControl 

This is pretty scary, but makes simplified if statements. If in any case you have many links to controls, I would prefer to use a code-based view of what loads are. You can wrap such an array in a class so that you can access the elements using something more:

 ControlClassInstance.single.truck 

You will have the following code:

 p1 = IsMultiPost ? ControlClassInstance.multi : ControlClassInstance.single p2 = p1[PostingType] //(this call would mean adding an indexer) 

Such a decision is too complicated if you do not expect something to get complicated ... and perhaps also be poor.

Answer 6

  singleLoadControl.Visible = false; singleTruckControl.Visible = false; multiTruckControl.Visible = false; multiLoadControl.Visible = false; singleLoadControl.Visible = (PostingType == PostingTypes.Loads && !IsMultiPost); singleTruckControl.Visible = (PostingType == PostingTypes.Trucks && !IsMultiPost); multiLoadControl.Visible = (PostingType == PostingTypes.Loads && IsMultiPost); multiTruckControl.Visible = (PostingType == PostingTypes.Trucks && IsMultiPost); 

Answer 7

Have you considered the status template ?

Answer 8

 private void ControlSelect() { if (PostingType == PostingTypes.Loads && !IsMultiPost) { singleLoadControl.Visible = true; singleTruckControl.Visible = false; multiTruckControl.Visible = false; multiLoadControl.Visible = false; return; } if (PostingType == PostingTypes.Trucks && !IsMultiPost) { singleLoadControl.Visible = false; singleTruckControl.Visible = true; multiTruckControl.Visible = false; multiLoadControl.Visible = false; return; } if (PostingType == PostingTypes.Loads && IsMultiPost) { singleLoadControl.Visible = false; singleTruckControl.Visible = false; multiTruckControl.Visible = false; multiLoadControl.Visible = true; return; } if (PostingType == PostingTypes.Trucks && IsMultiPost) { singleLoadControl.Visible = false; singleTruckControl.Visible = false; multiTruckControl.Visible = true; multiLoadControl.Visible = false; return; } }