Writing concise multiple else/if statement

Question

I have following code ,

@checkedin=[]
@accepted=[]
@rejected=[]   
result.each do |parse_order|
  orderId = parse_order['orderID']
    if parse_order['status'] == -1
        @rejected << orderId
    elsif parse_order['status'] == 1
        @accepted << [orderId, parse_order['createdAt']]
    elsif parse_order['status'] == 2
       @checkedin << [orderId, parse_order['createdAt']]
    elsif parse_order['status'] == 3
        next       
   end   
end

is there a better way to concise it . Thanks.

August · Accepted Answer · 2014-11-19 03:48:31Z

5

You could use single line conditionals. Also, next is unnecessary at the end of a block.

@checkedin = []
@accepted  = []
@rejected  = []   

result.each do |parse_order|
  orderId = parse_order['orderID']
  status = parse_order['status']

  @rejected  << orderId if status == -1
  @accepted  << [orderId, parse_order['createdAt']] if status == 1
  @checkedin << [orderId, parse_order['createdAt']] if status == 2
end

edited Nov 19, 2014 at 3:48

answered Nov 19, 2014 at 3:37

August

12.6k3 gold badges38 silver badges53 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Josh Bodah · Accepted Answer · 2014-11-19 03:40:57Z

3

@checkedin=[]
@accepted=[]
@rejected=[]   
result.each do |parse_order|
  orderId = parse_order['orderID']
  case parse_order['status']
  when -1 then @rejected << orderId
  when 1 then @accepted << [orderId, parse_order['createdAt']]
  when 2 then @checked_in << [orderId, parse_order['createdAt']]
  end
end

You don't need the last case

I'd recommend pulling out each of those cases for readability

result.each do |parse_order|
  reject(parse_order) || accept(parse_order) || check_in(parse_order)
end

def reject(parse_order)
  @rejected << parse_order['orderId'] if parse_order['orderId'] == -1
end

def accept(parse_order)
  @accepted << [parse_order['orderId'], parse_order['createdAt']] if parse_order['orderId'] == 1
end

def check_in(parse_order)
  @checked_in << [parse_order['orderId'], parse_order['createdAt']] if parse_order['orderId'] == 2
end

I'd probably rename parse_order to order for brevity too

edited Nov 19, 2014 at 3:40

answered Nov 19, 2014 at 3:33

Josh Bodah

1,2439 silver badges17 bronze badges

4 Comments

harshit Over a year ago

in above case does the arrays gets initialized at class level ?

Josh Bodah Over a year ago

No, you'd have to prefix everything with self. I think Pete's solution is syntactically best out of all the solutions posted

Cary Swoveland Over a year ago

I much prefer what you have at the top. You said you revised it for readability, but I think it's much less readable. That's mainly because we have years of experience reading horizontally (top), and have got quite good at it. Reading vertically (bottom) is just inefficient. Also, the more compact the code (within limits, of course), the more the eye can take in at once. When I look at your code on the top I know what you're doing almost instantly. On the bottom I've got to hunt around before I understand the big picture. In writing code, "read-speed" is one of my most important criteria.

Josh Bodah Over a year ago

That's fair. Everyone is different. I personally like to have the business rules very clear so I can quickly understand how the cases branch from one another, so I like those to be clean and concise. I usually don't care about the implementation of a branch unless I'm dealing with it specifically. I also like isolating my branches with their logic/state if possible (e.g. each branch method contains both its boolean check for state and the logic keeping the top layer more generic). I feel this makes refactoring to an OO model later easier for me.

Pete · Accepted Answer · 2014-11-19 04:22:23Z

I'm going to throw my hat in the ring. IMHO it is more human readable and would be easier to maintain. While it has more lines of code (due to the Order class), the actual filtering of orders is more concise:

require 'order' 
orders = results.map{|result| Order.new(result['status'], result['createdAt'])}

@checked_in_orders = orders.select {|order| order.checked_in?}
@accepted_orders = orders.select {|order| order.accepted?}
@rejected_orders = orders.select {|order| order.rejected?}

The Order class is:

# orders.rb
class Order
  REJECTED_STATUS = -1
  ACCEPTED_STATUS = 1
  CHECKED_IN_STATUS = 2

  attr_reader :created_at

  def initialize(status, created_at)
    @status, @created_at = [status, created_at]
  end

  def rejected?
    @status == REJECTED_STATUS
  end

  def accepted?
    @status == ACCEPTED_STATUS
  end

  def checked_in?
    @status == CHECKED_IN_STATUS
  end     
end

Cary Swoveland · Accepted Answer · 2014-11-19 06:09:36Z

2

I would use Enumerable#group_by:

REJECTED   = -1
ACCEPTED   =  1
CHECKED_IN =  2

h = result.group_by {|order| order['status']}
@rejected   = h[REJECTED  ].map {|order|  order['orderID']}
@accepted   = h[ACCEPTED  ].map {|order| [order['orderID'] order['createdAt']]}
@checked_in = h[CHECKED_IN].map {|order| [order['orderID'] order['createdAt']]}

edited Nov 19, 2014 at 6:09

answered Nov 19, 2014 at 4:29

Cary Swoveland

111k6 gold badges69 silver badges105 bronze badges

Comments

Paulo Fidalgo · Accepted Answer · 2014-11-19 09:11:08Z

0

You can use a case expression:

@checkedin=[]
@accepted=[]
@rejected=[]   
result.each do |parse_order|
  orderId = parse_order['orderID']

  case parse_order['status']
  when  -1
    @rejected << orderId
  when 1
    @accepted << [orderId, parse_order['createdAt']]
  when  2
    @checkedin << [orderId, parse_order['createdAt']]
  when 3
    next       
  end   
end

You can see more uses of case in this blog post and in the official documentation.

answered Nov 19, 2014 at 9:11

Paulo Fidalgo

22.4k7 gold badges105 silver badges121 bronze badges

Collectives™ on Stack Overflow

Writing concise multiple else/if statement

5 Answers 5

Comments

4 Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

Comments

4 Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related