This is a simple bit of code that I think is ugly. I am looking for a suggestion on making it more idiomatic. Thanks!

gistfile1.rb

  def populate_todolist
		progs = @user.participating_programs
		@to_do_list = progs.map { |prog| build_todolist_row(prog) }
		@to_do_list.compact!
	end

	# return nil if analysis yields no todolist row. compact! above removes the nil.
	def build_todolist_row prog
		rounds = prog.open_rounds
		raise "more than one round open in ToDoList" if rounds.length > 1
		if rounds.length == 1
			result = ToDoListRow.with(program_name: prog.name)
		end
		return nil
	end

def populate_todolist # revisit this name
  @to_do_list = @user.participating_programs.select(&:open?).map(&:build_to_do_list)
end

Since you're raising an exception if rounds.length > 1 do you need the if statement on line 11?

def build_todolist_row prog
  rounds = prog.open_rounds
  raise "more than one round open in ToDoList" if rounds.length > 1
  result = ToDoListRow.with(program_name: prog.name)
  return nil
end

Also, is there a reason to return nil?

Is ToDoListRow.with somehow mutating state somewhere? If you're just instantiating something and putting it in result, you're not returning result from that method - making line 12 a no-op.

Also, unless you use build_todolist_row somewhere else, do these need to be two methods? Something like:

def populate_todolist
  @todolist = @user.participating_programs.map do |program|
    rounds = program.open_rounds
    if rounds.length <= 1
      ToDoListRow.with(program_name: program.name)
    else
      raise "..."
    end
    rounds
  end
end

should work fine

Thanks! There was a bug due to a last minute untested tweak before I posted it so indeed the return null at the end is wrong.

Re: @arwegner: @to_do_list = @user.participating_programs.select(&:open?).map(&:build_to_do_list)
Looks promising, but the "open" method is on rounds which belong_to programs.

Re: @hurshawrawai: See my first sentence here. The reason I had two methods is that the logic for creating a ToDoListRow is going to get much more complex, and, in fact, sometime after deeper probing it might turn out that for a particular prog there is no to ToDoListRow needed.

Thanks!

Pito

@pitolas, i'm suggesting that it should be defined on programs, too. Or some method that encapsulates the notion of whether or not the program has more than one open round. Maybe the name isn't right; I don't know your domain.

I would change the API of these methods a little. Resulting in the following

def populate_todolist(programs)
  @to_do_list = programs.map do |program|
    build_todolist_row(program.name, program.open_rounds)
  end
end

def build_todolist_row(program_name, rounds)
  raise "more than one round open in ToDoList" if rounds.length > 1

  ToDoListRow.with(program_name: program_name)
end