none
Help with a design pattern for nested if statements

    Question

  •  

    Hi,

     

    Can anybody suggest a design pattern to replace nested if statements I have in my code?

     

    e.g

     

    public void MyMethod(string aString, string bString, bool aBool)

     

    if(ShopperIsValid(aString) & ShopperNameIsValid(bString))

    {

    if(DeviceIsValid(bString))

    {

    // do something

     

    if(CheckDevice(aBool))

    {

    if(ShopperAndDevice(aString, aBool))

    {

    RegisterShopper(aString, bString);

    }

    }

    }

    }

     

     

    I did try and refactor this using the State pattern but didnt seem to make much sense as the only states are valid or invalid shopper.

     

    Very grateful for any advice/help/pointers

     

     

     

    Thursday, September 18, 2008 9:43 PM

All replies

  • If the problem you're trying to solve has several rules and alternative flows like this, what you're really looking at is Forward or Backward Chaining, and these are more of techniques than patterns. Why don't you look at the rules engine in Windows Workflow Foundation instead?
    Saturday, September 20, 2008 2:45 PM
  • You may want to look at the State machie in WF (not WWF anymore that gets MS in trouble) but I thought I'd share a "rule" my old contractor boss enforced (a little too harshly IMO). It is, 'whenever you have a nested if you should put the whole block into a function'. Now it can work, and can make code easier to read. However, it does pose a problem in inventing suitably descriptive names though! Personally I'd go the rule route, be it WF or a collection of your own rules, much easier to configure rather than recompile for every change.

    Sunday, September 21, 2008 8:33 PM
  • If that is the only time you have those nested ifs, and that is how it is refactored, you can walk away. If you find those conditionals repeat, take another look at the state pattern. Two states is not too few to benefit from the value of having encapsulated states...

    Windows Workflow introduces much more complexity, and is probably an overkill in this situation.

    You mention that you only have two states, but you have a lot more conditional checks. What do the other functions do? Maybe post some more code here, and give a little background.

    You could conceivably create an object which represents a shopper (which is what i read the snippet to do). You can then set these values into the shopper, and it will in turn have an IsValid() method. Your code then might look like:
    Shopper shopper = new Shopper();
    shopper.Name = astring;
    shopper.WhatEver = bstring;
    if (shopper.IsValid())
    {
       RegisterShopper(shopper);
    }

    Or something along those lines. What you're doing there is encapsulating the conditionals into an object, which makes them reusable, and centralised.

    Wednesday, September 24, 2008 7:55 PM
  • Your code can be un-nested a little bit by using short circuited logical operators. For example:

     

    public void MyMethod(string aString, string bString, bool aBool)

    {

    if (ShopperIsValid(aString) && ShopperNameIsValid(bString) && DeviceIsValid(bString))

    {

    // do something

    if (CheckDevice(aBool) && ShopperAndDevice(aString, aBool))

    {

    RegisterShopper(aString, bString);

    }

    }

    }

     

    Any time you have a string of nested if statements you can consider them a string of "and" operators (as long as you don't have any else statements and as long as you don't have code in between).  Use short circuiting "and" operators to preserve the nested if behavior.

     

    -- Nathan

    Saturday, January 10, 2009 3:38 AM
  • Thanks for the reply.  Unfortunately, as you say, this solution won't work if you need to do any work

    in between the operators.

    Saturday, January 10, 2009 4:08 PM
  • The GoF Strategy pattern can used to reduce conditional statements by encapsulating algorithms. 

     

    see http://en.wikipedia.org/wiki/Strategy_pattern

     

    However, your example looks too simple to be worth the additional complexity.

     

    Hope this helps.

     

    Tuesday, January 13, 2009 11:45 AM
  • Just wondering if a Chain of Responsibility would be a good direction to go here. So have separate Shopper and Device classes and then have something like (very psuedo code);
    MyClass()
    {
       List{new Shopper}
    }

    MyMethod()
    {
    foreach(List.Check())
    }

    Shopper : IMyMethodChecker
    {
    Shopper()
    {
    List(new Device)
    }

    Check()
    {
    if IsValid()
    {
     foreach(List.Check())
    }
    }

    }

    Hopefully you get the idea. That way you could use some form of IoC and add/remove "checkers" without recompilation. Just a thought.

    http://pdkm.spaces.live.com/
    Sunday, June 07, 2009 5:10 PM
  • Consider combining some of your routines:

    bool ShopperIsValid(string aString, string bString)
    {
          // ShopperIdIsValid is old ShopperIsValid method. I'm assuming aString is shopper Id.
          return ShopperIdIsValid(aString) && ShopperNameIsValid(bString);
    }
    
    bool CheckShopperIdAndDevice(string aString, bool aBool)
    {
           return CheckDevice(aBool) && ShopperAndDevice(aString, aBool);
    }
    
    then your main routine becomes
    
    if(ShopperIsValid(aString, bString)
    {
        if(DeviceIsValid(bString)
        {
            if(CheckShopperIdAndDevice(aString, aBool))
            {
                RegisterShopper(aString, bString);
            }
         }
    }
    Hope this helps,
    Edwin Ames
    Monday, June 08, 2009 3:20 AM
  • Use a database?  If this check is always being used, and it makes sense to categorise each combination of checks into it's own attribute, you can select data for one or more of these attributes.

    If there are multiple similar implementations being asked the same question, then a strategy pattern is the way to go.  See www.dofactory.com for examples.

    Hope this helps,

    Martin.

    MCSD, MCTS. Please mark my post as helpful if you find the information good!
    Wednesday, June 24, 2009 5:45 AM