codereading / rack

a modular Ruby webserver interface
http://rack.rubyforge.org/
Other
18 stars 2 forks source link

Break complex conditional logic #4

Open samnang opened 12 years ago

samnang commented 12 years ago

I see a complex a conditional logic in https://github.com/codereading/rack/blob/rack-1.4/lib/rack/showexceptions.rb#L46

def prefers_plain_text?(env)
  env["HTTP_X_REQUESTED_WITH"] == "XMLHttpRequest" && (!env["HTTP_ACCEPT"] || !env["HTTP_ACCEPT"].include?("text/html"))
end

If I have a long conditional logic like this, I would break each part to make it easy to read:

def prefers_plain_text?(env)
  env["HTTP_X_REQUESTED_WITH"] == "XMLHttpRequest" && 
    !(env["HTTP_ACCEPT"] and env["HTTP_ACCEPT"].include?("text/html"))
end
jhuckabee commented 12 years ago

@samnang I agree that splitting the line makes it easier to read, however I think the original version of the 2nd conditinal logic reads more cleanly.

I.e.

(!env["HTTP_ACCEPT"] || !env["HTTP_ACCEPT"].include?("text/html"))

vs

!(env["HTTP_ACCEPT"] and env["HTTP_ACCEPT"].include?("text/html"))
  1. Keeping the NOT operator with each condition makes it flow easier when you read it.
  2. I find the use of && and and together inconsistent and distracting. Was that done on purpose?
samnang commented 12 years ago

Hi @jhuckabee

  1. Keeping the NOT operator with each condition makes it flow easier when you read it.

Sometimes it does that way, but this code it doesn't help because our brain will interpret more ! and ||, that's make it more complex.

  1. I find the use of && and and together inconsistent and distracting. Was that done on purpose?

I don't know most of the time I usually use and and or, but I use && and || whenever I think about operator precedence. So what do you think?