tags:

views:

442

answers:

8
+6  Q: 

Refactoring an If else tree

I have an if else tree that is going to grow as I add additional items for it to maintain and I'm looking at the best way to write it for maintainability I'm starting 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 thinking of 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 manage all of my controls is a single place

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

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

I don't take a performance hit but I'm maintaining the controls in multiple location

I'm leaning for option one just because I like maintainability aspect of it.

+23  A: 

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 capability to make multiple controls visible (or add more enumerated values) decorate the enum with [Flags] attribute as follows:

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

and modify 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 );
Charles Bretana  2009-09-03 17:14:46
This will still work. All of the values will be false except for one with the algorithm above.
JSBangs  2009-09-03 17:26:05
@JS is right. Only one will get set to visible as all the boolean expressions are exclusive
Charles Bretana  2009-09-03 17:33:07
It works for the scenario posted, but I feel this approach could prove hard to maintain should more enumeration values and controls be added, or combinations be required.If it never changes, however, I think this is a good, tidy refactoring.
Jeff Yates  2009-09-03 17:43:14
@Jeff, using [Flags] decorated enumeration allows for unlimited expansion with minimum maintenance headaches... but requires you understand bitwise logic ...
Charles Bretana  2009-09-03 17:52:06
@Charles: Yup, that's a reasonable idea and you could tidy that up further with an extension method for checking the values. Nice.
Jeff Yates  2009-09-03 17:54:12
@Jeff, I'm only just this month switching from 2.x to 3.x, so extension methods are new to me... Thx for the idea!
Charles Bretana  2009-09-03 17:55:50
I like the flags Idea
Bob The Janitor  2009-09-03 18:11:40
@Charles: I use them often for flags checking. I usually create Contains and ContainsAny; the first matches a given set of flags exactly against a value, the second checks if any of the bits in the given flags are set.
Jeff Yates  2009-09-03 20:15:15
+5  A: 

What 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;
}
Sergio  2009-09-03 17:18:22
This is certainly better than the OPs code, but I feel it is on the path to an even tighter refactoring as suggested by Charles Bretana, Chris Brandsma, or myself.
Jeff Yates  2009-09-03 17:47:42
+7  A: 

As you appear to be using an enumeration, I would recommend a switch with a default case to cope with unknown values. I believe this approach makes intentions clearer than doing all the checking 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.");
}
Jeff Yates  2009-09-03 17:23:43
+1 for code readability/clarity vs. clever assignments and bitwiseness (is that even a word!?).
Metro Smurf  2009-09-03 20:24:02
+1  A: 

If you are expecting to be adding a lot of different parameters, you could create a multi-dimensional 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 for simpler if statements. If you're going to have loads of references to the controls anyhow, I'd rather use a code-based representation of what those loads are. Such an array can be wrapped in a class to let you access the elements using something more like:

ControlClassInstance.single.truck

You'd have code like this:

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

This kind of solution is way too sophisticated unless you expect things to get complicated...and might be poor then, too.

Brian  2009-09-03 17:24:56
I'm not sure sophisticated is the right word for this. Obfuscated, unmaintainable, confusing, overkill; these words seem a better fit. While the use becomes clearer, the implementation is convoluted to the point that others may struggle to understand and maintain it, which could lead to easy bugs.
Jeff Yates  2009-09-03 17:44:56
@Jeff: Yeah, it does add way too much overhead. It would really only be justified if a lot of properties were being added and if the usage changes it fosters would bleed through into other places. But in that case, there's probably a better way. :::Shrug:::
Brian  2009-09-03 18:02:45
+1  A:  
  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);
Chris Brandsma  2009-09-03 17:33:31
There is no reason to initialize all the values to false as this will happen by the virtue of your later declarations.
Jeff Yates  2009-09-03 17:49:04
I was thinking of something like this, but then just using a switch statement to select the specific instance to enable, but Jeff Yate's answer works pretty much the same as that anyway (and is probably a little more elegant with that boolean in there)
Grant Peters  2009-09-03 23:44:20
Absolutely yes. But something in the post'ers answers made me think this was part of a lot more code. So I left them in.
Chris Brandsma  2009-09-04 00:13:05
+2  A: 

If it would be otherwise sensible (for example, if these are already domain-specific custom controls), you could 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 would be responsible for its own 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, in your page, just iterate over your IPostingControls and call SetVisibility(postingType, isMultiPost).

Jeff Sternal  2009-09-03 17:40:36
An interesting idea, but if the only reason to specialise is to add handling for this visibility logic, I don't think it's a good refactoring. I don't want to specialise TextBox's just to add specific code for handling when they should be visible - I don't even think every instance should need to know what state it's owner wants things.
Jeff Yates  2009-09-03 17:50:18
Agreed, this is overkill if the controls in question are just TextBoxes (or any other out-of-the-box UserControl), the domain isn't likely to change, and the logic is only in a single place. It might not even be worth it if only two of those are true. But if one or none of those are true, it definitely would be worth it!
Jeff Sternal  2009-09-03 17:54:07
this would tightly couple the control to this specific use
Bob The Janitor  2009-09-03 18:20:58
Not necessarily this specific use, but it would certainly tightly couple the control to the domain (where the concepts of `PostingType` and `IsMultiPost` are coherent), which is sometimes desirable.
Jeff Sternal  2009-09-03 18:44:57
when is tightly coupling something desirable? Necessary at time yes, but never desirable
Bob The Janitor  2009-09-03 22:13:31
I shouldn't have said 'coupling' since I really just meant expressing the domain. I'm talking about things like creating a WebControl that presents a drop-down list of all possible statuses for deliveries, which you could use throughout your application in several different contexts. Basically, creating a domain representation that helps you modularize the UI.
Jeff Sternal  2009-09-04 00:07:03
A:  
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;
    }
}
yigal  2009-09-03 18:20:04
this is exactly what I'm NOT trying to do
Bob The Janitor  2009-09-03 18:39:49
-1 this is a horrible suggestion, IMHO. It makes the code worse rather than better, introducing multiple opportunities for maintenance bugs.
Jeff Yates  2009-09-03 20:17:10
+1  A: 

Have you considered the State Pattern?

krdluzni  2009-09-03 19:03:19

related questions

Displaying Flash content in a C# WinForms application
How to get the value of built, encoded ViewState?
Unhandled Exception Handler in .NET 1.1
How do I connect to a database and loop over a recordset in C#?
How do I most elegantly express left join with aggregate SQL as LINQ query
Get a new object instance from a Type in C#
.NET Testing Framework Advice
Automatically update version number
What is the difference between an int and an Integer in Java/C#?
How to write to Web.Config in Medium Trust ?
WinForms ComboBox data binding gotcha
How do you sort a C# dictionary by value?
Adding Scripting functionality to .NET applications
Floating Point Number parsing: Is there a Catch All algorithm?
How do I print an HTML document from a web service?
Decoding T-SQL CAST in C#/VB.net
Anatomy of a "Memory Leak"
How do I get a distinct, ordered list of names from a DataTable using Linq
Reliable Timer in a Console Application
How do I fill a DataSet or a DataTable from a LINQ query resultset ?
What's the difference between Math.Floor() and Math.Truncate() in .NET?
How do I calculate relative time?
How do I calculate someone's age in C#?
Are there any conversion tools for porting Visual J# code to C#?
When setting a form's opacity should I use a decimal or double?