still more relentless non-repetition

Discussion in 'Ruby' started by Giles Bowkett, Oct 31, 2006.

  1. ok, I have this Rails code which I want to make more Rubyish.

    controller:

    def tracks
    @term = params[:term] || ''
    if @term.blank?
    @tracks = []
    else
    @tracks = Track.find_by_contents @term
    end
    end

    view:

    <% for word in %w{ tracks beats users profiles } %>

    <% unless controller.action_name == word %>
    <%= link_to "search similar #{word}", :action => word, :term => @term %>
    <br/>
    <% end %>

    <% end %>

    now this is using Ferret, the Ruby port of Lucene, via the
    acts_as_ferret Rails plugin.

    (by the way, Ferret actually runs faster than Lucene, according to the
    Ferret site. I don't know how, but that's pretty cool.)

    anyway, as you can see, I have one method in the controller and a view
    which assumes the existence of three additional methods.

    what I want to do is have only one method in the controller, which
    does all the searching.

    now I could do:

    def tracks
    @term = params[:term] || ''
    case params[:thing_to_search_for]
    when "tracks"
    if @term.blank?
    @tracks = []
    else
    @tracks = Track.find_by_contents @term
    end
    when "beats"
    if @term.blank?
    @beats = []
    else
    @beats = Beat.find_by_contents @term
    end
    when "profiles"
    if @term.blank?
    @profiles = []
    else
    @profiles = Profile.find_by_contents @term
    end
    when "users"
    if @term.blank?
    @users = []
    else
    @users = User.find_by_contents @term
    end
    end
    end

    however, this is what Rails users call "wet" and everybody else calls "stupid."

    what I want to do is a properly elegant thingy. here's roughly what I envision:

    def search
    @term = params[:term] || ''
    instance_variable_set("@#{params[:thing_to_search_for]}", [])
    unless @term.blank?
    eval("@#{params[:thing_to_search_for]}") =
    (eval(params[:thing_to_search_for].capitalize)).find_by_contents @term
    end
    end

    I think I should probably just start coding in Lisp and save everyone
    the irritation, but in the meantime, how does that look, in terms of a
    tree to bark up? that is to say, does it look like the right tree, or
    the wrong tree?

    if you can puzzle it through, I think what I have here is an elegant
    algorithm expressed in very-much-other-than-elegant code. (but I could
    be wrong!)

    --
    Giles Bowkett
    http://www.gilesgoatboy.org
    Giles Bowkett, Oct 31, 2006
    #1
    1. Advertising

  2. I'm too lazy ATM to read the whole thing and make a design
    recommendation, but Danger, Will Robinson!
    > eval("@#{params[:thing_to_search_for]}") =
    > (eval(params[:thing_to_search_for].capitalize)).find_by_contents @term

    Major Ruby-injection problem here. NEVER eval something you get from an
    untrusted user. Use, instead, instance_variable_get and Object.const_get.

    Devin
    Devin Mullins, Oct 31, 2006
    #2
    1. Advertising

  3. ah yeah, that's a good point, SQL injection attacks.

    On 10/31/06, Devin Mullins <> wrote:
    > I'm too lazy ATM to read the whole thing and make a design
    > recommendation, but Danger, Will Robinson!
    > > eval("@#{params[:thing_to_search_for]}") =
    > > (eval(params[:thing_to_search_for].capitalize)).find_by_contents @term

    > Major Ruby-injection problem here. NEVER eval something you get from an
    > untrusted user. Use, instead, instance_variable_get and Object.const_get.
    >
    > Devin
    >
    >



    --
    Giles Bowkett
    http://www.gilesgoatboy.org
    Giles Bowkett, Oct 31, 2006
    #3
  4. Giles Bowkett

    Guest

    Hi --

    On Wed, 1 Nov 2006, Giles Bowkett wrote:

    > On 10/31/06, Devin Mullins <> wrote:
    >> I'm too lazy ATM to read the whole thing and make a design
    >> recommendation, but Danger, Will Robinson!
    >> > eval("@#{params[:thing_to_search_for]}") =
    >> > (eval(params[:thing_to_search_for].capitalize)).find_by_contents @term

    >> Major Ruby-injection problem here. NEVER eval something you get from an
    >> untrusted user. Use, instead, instance_variable_get and Object.const_get.

    >
    > ah yeah, that's a good point, SQL injection attacks.


    It's not so much SQL injection as eval injection. Imagine if
    params[:thing_to_search_for] is "a=1; system('rm -rf /*')" or
    something. You'd be eval'ing the string:

    @a=1; system('rm -rf /*')


    David

    --
    David A. Black |
    Author of "Ruby for Rails" [1] | Ruby/Rails training & consultancy [3]
    DABlog (DAB's Weblog) [2] | Co-director, Ruby Central, Inc. [4]
    [1] http://www.manning.com/black | [3] http://www.rubypowerandlight.com
    [2] http://dablog.rubypal.com | [4] http://www.rubycentral.org
    , Nov 1, 2006
    #4
  5. Giles Bowkett

    Guest

    On Wed, 1 Nov 2006, Giles Bowkett wrote:

    > ok, I have this Rails code which I want to make more Rubyish.
    >
    > controller:
    >
    > def tracks
    > @term = params[:term] || ''
    > if @term.blank?
    > @tracks = []
    > else
    > @tracks = Track.find_by_contents @term
    > end
    > end


    do you really want to search twice here if tracks is called twice?

    i'm guessing you'd want

    def tracks
    return @tracks unless defined? @tracks
    t = params[:term]
    @tracks =
    if t.nil? or t.blank?
    []
    else
    Track.find_by_contents t
    end
    end

    > def tracks
    > @term = params[:term] || ''
    > case params[:thing_to_search_for]
    > when "tracks"
    > if @term.blank?
    > @tracks = []
    > else
    > @tracks = Track.find_by_contents @term
    > end
    > when "beats"
    > if @term.blank?
    > @beats = []
    > else
    > @beats = Beat.find_by_contents @term
    > end
    > when "profiles"
    > if @term.blank?
    > @profiles = []
    > else
    > @profiles = Profile.find_by_contents @term
    > end
    > when "users"
    > if @term.blank?
    > @users = []
    > else
    > @users = User.find_by_contents @term
    > end
    > end
    > end


    def tracks
    return @tracks unless defined? @tracks
    t = params[:term]
    @tracks =
    if t.nil? or t.blank?
    []
    else
    model = self.class.const_get t.capitalize
    model.find_by_contents t
    end
    end


    __never__ use eval on user data in webaps.

    regards.

    -a
    --
    my religion is very simple. my religion is kindness. -- the dalai lama
    , Nov 1, 2006
    #5
  6. > >> Major Ruby-injection problem here. NEVER eval something you get from an
    > >> untrusted user. Use, instead, instance_variable_get and Object.const_get.

    > >
    > > ah yeah, that's a good point, SQL injection attacks.

    >
    > It's not so much SQL injection as eval injection. Imagine if
    > params[:thing_to_search_for] is "a=1; system('rm -rf /*')" or
    > something. You'd be eval'ing the string:
    >
    > @a=1; system('rm -rf /*')


    true, that would also be very bad.

    --
    Giles Bowkett
    http://www.gilesgoatboy.org
    Giles Bowkett, Nov 1, 2006
    #6
  7. > model = self.class.const_get t.capitalize
    > model.find_by_contents t


    **that** looks like the way to do it.

    gracias!

    --
    Giles Bowkett
    http://www.gilesgoatboy.org
    Giles Bowkett, Nov 1, 2006
    #7
  8. Giles Bowkett

    Guest

    Hi --

    On Wed, 1 Nov 2006, wrote:

    > On Wed, 1 Nov 2006, Giles Bowkett wrote:
    >
    >> ok, I have this Rails code which I want to make more Rubyish.
    >>
    >> controller:
    >>
    >> def tracks
    >> @term = params[:term] || ''
    >> if @term.blank?
    >> @tracks = []
    >> else
    >> @tracks = Track.find_by_contents @term
    >> end
    >> end

    >
    > do you really want to search twice here if tracks is called twice?
    >
    > i'm guessing you'd want
    >
    > def tracks
    > return @tracks unless defined? @tracks
    > t = params[:term]
    > @tracks =
    > if t.nil? or t.blank?


    nil is considered blank by blank?, so you only need the one test.


    David

    --
    David A. Black |
    Author of "Ruby for Rails" [1] | Ruby/Rails training & consultancy [3]
    DABlog (DAB's Weblog) [2] | Co-director, Ruby Central, Inc. [4]
    [1] http://www.manning.com/black | [3] http://www.rubypowerandlight.com
    [2] http://dablog.rubypal.com | [4] http://www.rubycentral.org
    , Nov 1, 2006
    #8
  9. Giles Bowkett

    Guest

    On Wed, 1 Nov 2006 wrote:

    >> i'm guessing you'd want
    >>
    >> def tracks
    >> return @tracks unless defined? @tracks
    >> t = params[:term]
    >> @tracks =
    >> if t.nil? or t.blank?

    >
    > nil is considered blank by blank?, so you only need the one test.


    just putting my abhorrence of container methods defined on non-containers on
    public display ;-)

    -a
    --
    my religion is very simple. my religion is kindness. -- the dalai lama
    , Nov 1, 2006
    #9
  10. On 10/31/06, Giles Bowkett <> wrote:
    > > model = self.class.const_get t.capitalize
    > > model.find_by_contents t

    >
    > **that** looks like the way to do it.
    >
    > gracias!
    >


    ActiveSupport has a helper for this already.

    model = t.constantize
    model.find_by_contents(t)
    Wilson Bilkovich, Nov 1, 2006
    #10
  11. wow that's handy:

    thing.constantize.find_by_contents(term)

    boom! done.

    On 10/31/06, Wilson Bilkovich <> wrote:
    > On 10/31/06, Giles Bowkett <> wrote:
    > > > model = self.class.const_get t.capitalize
    > > > model.find_by_contents t

    > >
    > > **that** looks like the way to do it.
    > >
    > > gracias!
    > >

    >
    > ActiveSupport has a helper for this already.
    >
    > model = t.constantize
    > model.find_by_contents(t)
    >
    >



    --
    Giles Bowkett
    http://www.gilesgoatboy.org
    Giles Bowkett, Nov 1, 2006
    #11
  12. OK. here's what I have.

    def index
    @term = params[:term] || ''
    @category = params[:category] || ''
    if not %w{ Track Beat User Profile }.include? @category
    redirect_to "/"
    else
    instance_variable_set("@#{@category.downcase.pluralize}",
    (@category.constantize.find_by_contents @term))
    end
    end

    index view:

    <%= render_partial @category.downcase.pluralize %>

    <%= render_partial "options" %>

    options partial:

    <% for word in %w{ tracks beats users profiles } %>

    <% if eval("@#{word}").nil? %>
    <%= link_to "search similar #{word}",
    :action => "index",
    :term => @term,
    :category => word.capitalize.singularize %>
    <br/>
    <% end %>

    <% end %>





    the code in the model-specific partials is design-specific, but it
    picks up the instance vars, which is all that matters in this context.

    obviously I am using eval and instance_variable_set in ways I'm not
    supposed to, but in both cases they're filtered in such a way as to
    prevent harm.

    actually, if that still seems risky to anybody, I would like to know.
    I think it's probably OK but I could easily be guilty of
    overconfidence.

    I don't like all the upcasing and downcasing. it's not as elegant as
    it could be.

    what I do like about it is that it generates a lot of HTML without
    very much Ruby at all. that's pretty good bang for the buck. in one
    short method and two quick partials, you have all the code necessary
    to generate four different types of model-specific search pages, plus
    links for searching different models with the same query.

    obviously the array of words could be an instance var, and could be
    aribtrarily large, but only if I fixed the upcasing and downcasing.

    --
    Giles Bowkett
    http://www.gilesgoatboy.org
    Giles Bowkett, Nov 1, 2006
    #12
  13. Giles Bowkett

    Max Muermann Guest

    >
    > I don't like all the upcasing and downcasing. it's not as elegant as
    > it could be.
    >
    > what I do like about it is that it generates a lot of HTML without
    > very much Ruby at all. that's pretty good bang for the buck. in one
    > short method and two quick partials, you have all the code necessary
    > to generate four different types of model-specific search pages, plus
    > links for searching different models with the same query.
    >
    > obviously the array of words could be an instance var, and could be
    > aribtrarily large, but only if I fixed the upcasing and downcasing.
    >
    > --
    > Giles Bowkett
    > http://www.gilesgoatboy.org
    >


    Try:

    "Track".tableize => 'tracks'
    'tracks'.classify => Track

    and

    "MyCategory".tableize => "my_categories"
    "my_categories".classify => "MyCategory"

    Cheers,
    Max
    Max Muermann, Nov 1, 2006
    #13
  14. --------------enigAC890B69156F3593D79D893C
    Content-Type: text/plain; charset=ISO-8859-1
    Content-Transfer-Encoding: quoted-printable

    Giles Bowkett wrote:
    > (by the way, Ferret actually runs faster than Lucene, according to the
    > Ferret site. I don't know how, but that's pretty cool.)
    >=20


    A) All benchmarks lie, -especially- ones people use to pimp their own
    products.

    I'm not claiming this is the cause though, because:
    B) Ferret uses a C extension, and the benchmark is probably that
    version, not the pure Ruby one; I imagine advanced fulltext search
    algorithms do quite a fair share of numbercrunching, where C would win
    out. Of course, with Ruby it's entirely possible marshalling of results
    between C and Ruby would kill the pure performance gain (speaking
    generally, not of specifically Ferret anymore), but it's possible to
    skew a benchmark so that doesn't show - a large dataset and a small
    result set for an extension that instead tends to be used frequently for
    larger result sets. A full processor usage profile on datasets and
    result sets of varying sizes would tell books above a simple benchmark.

    David Vallner


    --------------enigAC890B69156F3593D79D893C
    Content-Type: application/pgp-signature; name="signature.asc"
    Content-Description: OpenPGP digital signature
    Content-Disposition: attachment; filename="signature.asc"

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.5 (MingW32)

    iD8DBQFFSmEKy6MhrS8astoRAkc9AJ4gBW1zs+Xq+4wlYnzcWRB3rl6KKwCbBMMo
    ciD7d8P4o4hvVLcL53il4p8=
    =4LN/
    -----END PGP SIGNATURE-----

    --------------enigAC890B69156F3593D79D893C--
    David Vallner, Nov 2, 2006
    #14
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Replies:
    5
    Views:
    352
  2. artem

    permutation with repetition

    artem, Feb 23, 2007, in forum: Java
    Replies:
    6
    Views:
    2,109
  3. bullockbefriending bard

    Generator for k-permutations without repetition

    bullockbefriending bard, Jul 4, 2007, in forum: Python
    Replies:
    4
    Views:
    1,035
    =?ISO-8859-1?Q?Nis_J=F8rgensen?=
    Jul 4, 2007
  4. Jhovarie
    Replies:
    0
    Views:
    431
    Jhovarie
    Jan 13, 2011
  5. Shawn W_
    Replies:
    2
    Views:
    171
    Peter Szinek
    Jan 26, 2007
Loading...

Share This Page