1
\$\begingroup\$

In my rails controllers, I am consistently using this logic:

def report
  dump = params[:dump] ? params[:dump] : 'false'

  data = {}

  if dump != 'false'
    return render :json => p(data)
  end
end

Is there a better way to write this? A ternary operation is just an eyesore. I thought maybe just using the || operator:

dump = params[:dump] || false

But that doesn't yield the desired result.

How can I better write this?

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Those look like placeholder names, and "this logic" could also imply that you're just showing an example. We require real code that you're working on. \$\endgroup\$
    – Jamal
    Nov 11, 2014 at 21:32
  • \$\begingroup\$ Not only that, we'd like to see the whole controller method, so that we can understand why you would want to set it to false. \$\endgroup\$ Nov 11, 2014 at 21:34
  • \$\begingroup\$ Edited. Thanks. I don't mind editing questions, but when closing, it'd be nice to have a comment on why so i can improve on my question :) \$\endgroup\$
    – ddavison
    Nov 11, 2014 at 21:36
  • \$\begingroup\$ Sorry. Thanks for your help to make it a better question. \$\endgroup\$ Nov 11, 2014 at 21:38
  • \$\begingroup\$ This is my first code-review question. I'm a regular on SO.. the "How to ask" seems to be unclear about "masked" variables. I thought putting masked(renamed) variables would have been fine, but I understand why the context would be important. \$\endgroup\$
    – ddavison
    Nov 11, 2014 at 21:39

4 Answers 4

3
\$\begingroup\$

I'm unclear how you are using dump = false and whether the method returns anything more than nil when params[:dump] is unassigned. If it doesn't, I would recommend the use of a guard clause:

def report
  return unless params[:dump]
  data = {}
  render :json => p(data)
end
\$\endgroup\$
0
\$\begingroup\$

You may use the coalesce gem as mentioned in this SO answer.

def report
    dump = params[:dump]._? false

    data = {}

    if dump != false
        return render :json => p(data)
    end
end

Though I believe it would be more idiomatic to just use the falsyness of nil like this:

def report
    dump = params[:dump]
    data = {}

    if dump
        return render :json => p(data)
    end
end
\$\endgroup\$
0
\$\begingroup\$

This is a common pattern in Rails, so common that there is a Ruby Objct extension for it called presence.

Returns the receiver if it's present otherwise returns nil. object.presence is equivalent to

object.present? ? object : nil

In your case, the code will be changed from

dump = params[:dump] ? params[:dump] : 'false'

to

dump = params[:dump].presence || false

It's hard to refactor the entire code, because it's not clear what should happen in case the parameter is false. I assume you may want to display an error.

If that's the case, there is also another way: use an exception, or even better, reuse require from the Rails strong parameters feature, which is based on exceptions

dump = params.require(:dump)

This line alone, will allow you to get rid of the entire body.

def report
  dump = params.require(:dump)

  data = {}
  render :json => p(data)
end

If the parameter is missing, an exception ActionController::ParameterMissing is raised. You can rescue it in the app, and show the proper message.

rescue_from ActionController::ParameterMissing, with: :handle_parameter_missing

def handle_parameter_missing(exception)
  # do what you want
end
\$\endgroup\$
1
  • \$\begingroup\$ this looks promising. i'll try this \$\endgroup\$
    – ddavison
    Nov 14, 2014 at 14:17
0
\$\begingroup\$

Pure ruby is your friend here :

dump = params.fetch(:dump, false)

see the docs for more info.

note : If you get an emtpy string from your params, in this case I usually find it more relevant to change the markup to avoid the need for .presence calls. This is a matter of taste ; all things considered, in ruby any object other than nil and false is truthy, so I think it is coherent with ruby's 'spirit' to avoid passing meaningless params at all.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.