CODEWITHSUNDEEP
refactoringconditional-statements
domi91c
domi91c

Reputation: 2053

Refactoring large conditional-statements

Is there a better way to write conditional logic such as this?

if (dog == 'alive' && cat == 'alive')
    ...
elsif (dog != 'alive' && cat == 'alive')
    ...
elsif (dog == 'alive' && cat != 'alive')
    ...    
elsif (dog != 'alive' && cat != 'alive')
    ...    
end

If I were to add another animal, or another 4, things would get pretty out of hand. There must be a better way!

EDIT: My actual code has nothing to do with cats and dogs. I was just trying to keep it as general as possible. Not sure if this will affect the answers, but here you go. (It's Rails.) 

    if params[:search]
        @requests = Request.search(params[:search], @cookies_city, @cookies_category).order('created_at DESC')
    elsif cookies[:city_select]
        if @cookies_city != "ALL" && @cookies_category != "ALL"

            @requests = Request.all.where(:city => @cookies_city).where(:category => @cookies_category).order('created_at DESC')
        elsif @cookies_city != "ALL" && @cookies_category == "ALL"
            @requests = Request.all.where(:city => @cookies_city).order('created_at DESC')

        elsif @cookies_city == "ALL" && @cookies_category != "ALL"
            @requests = Request.all.where(:category => @cookies_category).order('created_at DESC')

        elsif @cookies_city == "ALL" && @cookies_category == "ALL"

            @requests = Request.all.order('created_at DESC')
        end
    elsif cookies[:category_select]
        if @cookies_city != "ALL" && @cookies_category != "ALL"
            @requests = Request.all.where(:city => @cookies_city).where(:category => @cookies_category).order('created_at DESC')

        elsif @cookies_city != "ALL" && @cookies_category == "ALL"
            @requests = Request.all.where(:city => @cookies_city).order('created_at DESC')

        elsif @cookies_city == "ALL" && @cookies_category != "ALL"
            @requests = Request.all.where(:category => @cookies_category).order('created_at DESC')

        else
            @requests = Request.all.order('created_at DESC')

        end
    end
end

Upvotes: 1

Views: 282

Answers (4)

Ira Baxter
Ira Baxter

Reputation: 95354

Your original coding style is generally called a Decision Table (DT).

DT's are good, because it is easy to modify the conditions under which each action is taken easily.

When you hand code, they look big and clunky like you have. (Why do you care?)

One objection might be that the code isn't efficient (does it matter for your particular case?) because it re-evaluates the sub-conditions repeatedly. That may or may not be true; its your compiler's optimizing ability that decides that; a good compiler can pick out useful subexpressions and evaluate a small set.

For performance, one might want to have a decision tree. The good news is they are fast; the bad news is they are hard to update. If you insist on a decision tree, it is best you write your tests as a decision table for maintainability, and let some tool convert the conditions into the decision tree for you. Then you end up with a maintainable result, and fast performance.

Upvotes: 1

Carl Manaster
Carl Manaster

Reputation: 40336

If you can structure your code so that you return from each branch, it can be a lot cleaner. Sometimes this may entail extracting a method. And ternary operators, if your language supports them, can make matters clearer if used judiciously

if (dog == 'alive')
  return cat == 'alive' ? 'bothLiving' : 'justDog';

return cat == 'alive' ? 'justCat' : 'bothDead';

Upvotes: 1

RadioSpace
RadioSpace

Reputation: 952

First you should derive all those from a common class like public class Animal {public bool Alive{get; set;}}.

put them all in a 'List'

then run them through 2 for loops (pseudo code)

for(int x = 0;x < animals.Count;x++)
{
   for(int y = 0;y < animals.Count;y++)
   {
     if(animals[x].Alive == animals[y].Alive)....
   }
}

sorry but that's as far as can could go with this answer since you are looking for a means without an end

I am not really sure how you would collect and use the data of comparing each animals alive status to each others but this should get you started

and if you don't want to compare the same animals just test if x and y are the same

Upvotes: 0

rajpal gusain
rajpal gusain

Reputation: 81

You may use different bits to represent animal's state (alive or dead). Then it's a simple switch statement like

switch (state) {
case 0: // None alive
...
break;
case 1: // 0th bit alive only(say dog)
...
break;
case 2: // 1st bit alive only (say cat)
....
break;
case 3: // 0th and 1st bit alive (dog and cat)
}

And when you want to add more just use next bit and add 'case' inside switch statement.

Upvotes: 1

Related Questions