Ruby on Rails: If you have 50 if-else statements in your after_create action, will that slow down your application?

Question

Is using 50 if-else statements too resource-intensive for one action?

I'm doing something like this:

if team.players.count > 1
   assign_team_type(..)
elsif team.players.count > 3
   assign_team_type(..)
...
etc.
...
end

Also, is it more efficient to place the 50 if-else statements in your create action inside your controller instead of the after_create method? Or would it be more efficient to use a case switch statement instead, or just avoid it altogether?

EDIT: Thanks for the very quick responses! The code is for a community sports tournament to assign teams based on the number of players on that team. I'm trying to write something that assigns a team type to each team according to how many players are added to that team. So there are teams for 1 player, 3 players, 5 players, 7 players, etc., up to 200 players, which requires 50 if-else statements in total.

The statements happen in the players_controller, after the user visits http://localhost/players/new, adds a player, and then the application decides what team to assign his or her team based on how many players are currently on that team. It's very straight-forward (a basic CRUD application that just needs these 50 if-else statements)

models:

Team (has_many :players)
Player (belongs_to :team)

scaffold team name:string team_type:string
scaffold player team_id:integer name:string

That's pretty much it :)

Answer 1

How about assigning each function to hash / dict?

d={1:assign_team_type(1),2:assign_team_type(2)}

Answer 2

Ugh, this is a very bad code smell. You likely have some substantial duplication that can be pushed down into the Model or, in the least, rolled up into something else.

Why not put them into an array or hash? Something like:

TeamTypes = { 1 => something, 2 => something_else, .. }

assign_team_type( TeamTypes[team.players.count] )

Answer 3

You could try to rewrite it as

assign_team_type(case team.players.count
                 when 2    then ...
                 when 3..5 then ...
                 else raise "Assignment failed"
                 end
)

which should probably be faster as team.players.count is evaluated only once. Additionally it is cleaner and shorter. A benchmark will help.

Answer 4

Based on your new info I suggest to add a max_players column to your team_types model and ask the model for something like this:

find(:first, :conditions => ["max_players < ?", self.player_count], :order => "max_players ASC")

This makes it totally dynamic and you can manage the team type assignment through the web interface.

Answer 5

My first instinct is to pack your data into a 2D array like:

x=[[1, "arg 1"],
   [3, "arg 2"],
   [9, "arg 3"],
   ...
  ]

where the first column is the team count and the second is the parameter that you would pass to assign_team_type.

You can search through the table for the argument you want using something like:

func_arg = x.collect{|w| w[0] < team.players.count ? w[1] : nil}.compact.last
assign_team_type(func_arg)

I admit it's kind of a C-like approach, but it will let you change your grouping parameters on the fly by simply modifying the array.

Answer 6

modern processors are capable of "branch prediction"

when there are if-else's to parse the processor will make an attempt to guess which one it'll need, if it guesses wrong that can slow down your app.

i would recommend refactoring any overly large branching sections into nice clean switch statements.