Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a 'fallback' argument to Model#get #2144

Closed
wants to merge 2 commits into from

Conversation

shiloa
Copy link

@shiloa shiloa commented Jan 16, 2013

Motivation

Sometimes it's useful to retrieve some default value from a Model, even if that value doesn't exist in the db (or not even an actual model attribute, if waiting implementation).

This fallback value isn't persisted, but rather acts as a sane default when the actual value returns true on _.isEmpty( value ). I found it useful in view templates, when I don't want to inline a lot of ternary logic like:

<%= @user.get('name') || 'Anonymous' %>

<!-- compared to -->

<%= @user.get('name', 'Anonymous') %>

It is basically equivalent to Python's dict.get(key, default=<something>) or Ruby's Hash#fetch(key, [default]). I do realize Model#get is a core method, so I'm not taking this addition lightly.

I've searched existing issues, but couldn't find something similar. Apologies in advance if I missed one. I did my best to stick to the guidelines, so hopefully things are in order.

Thanks!

@gsamokovarov
Copy link
Contributor

JavaScript has some pretty common idioms for those cases - just use this.user.get('name') || 'Anonymous') if you want to avoid the this.user.has call.

For CoffeeScript you can use the above idiom or (@user.get 'name') ? 'Anonymous' and it will even test against existence. This is what Model#has do anyway.

EDIT:

BTW @user.has('name') ? @user.get('name') : 'Anonymous' is not what you expect it to be 😄 CoffeeScript does not have the ternary operator you are used to.

@shiloa
Copy link
Author

shiloa commented Jan 16, 2013

@gsamokovarov I agree. But this, in my mind, makes for cleaner syntax on that particular method. I was surprised this wasn't already implemented 😉.

I think using something like this will reduce surprises in the long run for several cases, such as:

  • when a field isn't present or not yet implemented (stubbed out for design, templating, etc).
  • when a field isn't enforced or had it's validation removed.

In the grand scheme of things, it's not a killer feature but rather syntactic sugar that I find quite convenient.

Also, thanks for the EDIT @gsamokovarov - I've updated my pull request's example with yours.

@gsamokovarov
Copy link
Contributor

For the cases you specify I think you can go better with using Model#default. I can see your point, if you are coming from Python (I for one am), but its better to follow JS/CS principles here.

The dict.get(key, default) has the nasty disadvantage that it actually evaluates default all the time. While constructs like obj.key || default can evaluate it only when needed. I actually find myself using dict.get(key, default) less and less even in Python.

@shiloa
Copy link
Author

shiloa commented Jan 16, 2013

@gsamokovarov I guess this is true if performance is of great importance. If one is performing a lot of model.get(field, fallback) in the page, then it might degrade some.

However, I did give some thought to short-circuiting the evaluation of the fallback var - it's only called when the original attribute isn't present (not including the stack allocation for the args to the function).

I'm no JS expert - certainly in malloc/GC matters - so I'm sure a better implementation could be considered.

@braddunbar
Copy link
Collaborator

Mornin' @shiloa! Thanks for the patch. There is some precedent in #1184 and #1634 and I think the reasons there are still valid. While parity between different languages is nice, I think that using model.get(...) || '...' is a simpler, more idiomatic way to accomplish this.

@braddunbar braddunbar closed this Jan 16, 2013
@shiloa
Copy link
Author

shiloa commented Jan 16, 2013

Hmm, missed those. Thanks anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants