Forum: Ruby ugly ruby code...

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
49ba272c5adeec974d54990765672738?d=identicon&s=25 Arnaud S. (Guest)
on 2006-06-13 10:32
Hello!

When I look at this method and when I think to pursuit of beauty thanks
Ruby, I think I could do a nicer bunch of code...

I would like to seperate racks already added in the showed room and the
others
Here is the only method I could find but as a beginner I surely miss
some tricks!

Please, can someone give some hints?

[code]

def show
  @room = Room.find(params[:id])
  @racks = Rack.find( :all )
  @racks_room = Array.new
  @free_racks = Array.new

  for rack in @racks do
    if rack.room_id == @room.id
      @room_s_racks << rack
    else
      @free_racks << rack
    end
  end
end

[/code]
Robin S. (Guest)
on 2006-06-13 10:42
(Received via mailing list)
arnaud stageman wrote:
> end
The above code could be replaced by this:

   @room_s_racks, @free_racks = @racks.partition { |rack|
     rack.room_id == @room.id
   }

Partition can be very useful sometimes :)

Regards,
   Robin
49ba272c5adeec974d54990765672738?d=identicon&s=25 Arnaud S. (Guest)
on 2006-06-13 11:01
Great! It's working perfectly!
Hummm Ruby :)

Robin S. wrote:
> arnaud stageman wrote:
>> end
> The above code could be replaced by this:
>
>    @room_s_racks, @free_racks = @racks.partition { |rack|
>      rack.room_id == @room.id
>    }
>
> Partition can be very useful sometimes :)
>
> Regards,
>    Robin
Kroeger, Simon (ext) (Guest)
on 2006-06-13 11:38
(Received via mailing list)
> -----Original Message-----
> From: removed_email_address@domain.invalid
> [mailto:removed_email_address@domain.invalid] On Behalf Of arnaud stageman
> Sent: Tuesday, June 13, 2006 10:33 AM
> To: ruby-talk ML
> Subject: ugly ruby code...
>
> I would like to seperate racks already added in the showed
> room and the others
> [...]

This looks like active record, right?

what about

def show
  @room = Room.find(params[:id])
  @racks_room = Rack.find(:all, :conditions => "room_id = #{@room.id}")
  @free_racks = Rack.find(:all, :conditions => "room_id != #{@room.id}")
end

cheers

Simon
49ba272c5adeec974d54990765672738?d=identicon&s=25 Arnaud S. (Guest)
on 2006-06-13 11:46
Kroeger, Simon (ext) wrote:
>> -----Original Message-----
>> From: removed_email_address@domain.invalid
>> [mailto:removed_email_address@domain.invalid] On Behalf Of arnaud stageman
>> Sent: Tuesday, June 13, 2006 10:33 AM
>> To: ruby-talk ML
>> Subject: ugly ruby code...
>>
>> I would like to seperate racks already added in the showed
>> room and the others
>> [...]
>
> This looks like active record, right?
>
> what about
>
> def show
>   @room = Room.find(params[:id])
>   @racks_room = Rack.find(:all, :conditions => "room_id = #{@room.id}")
>   @free_racks = Rack.find(:all, :conditions => "room_id != #{@room.id}")
> end
>
> cheers
>
> Simon

It would be better :
def show
  @room = Room.find(params[:id])
  @racks_room = Rack.find(:all, :conditions => ["room_id = ?",
@room.id])
  @free_racks = Rack.find(:all, :conditions => ["room_id != ?",
@room.id])
end

Because you avoid sql injection.

The first solution is better because you execute only one sql request.
Robin S. (Guest)
on 2006-06-13 12:10
(Received via mailing list)
arnaud stageman wrote:
>       @free_racks << rack
>     end
>   end
> end

Hi again,

Just some minor suggestions:

You seem to use only instance variables. If you are not going to need
them later in views, I would prefer using local variables because they
go out of scope when the method finishes and don't clutter the object
with unnecessary variables. So, just replace @racks with racks.

Replace the long Array.new with the shorter form []. Of course this is
just my opinion, but I find it easier to type and easier to read as it
gives you some kind of visual clue that there's an array involved.

...my 0.02 Swiss francs ;)
Peter E. (Guest)
on 2006-06-13 12:23
(Received via mailing list)
Maybe like that:

---

class Room < ActiveRecord:Base
  has_many :racks
end

class Rack < ActiveRecord:Base
  # uses self.room_id for reference
  belongs_to :room
end

---

used_racks = Room.find(params[:id])racks
free_racks = Racks.find_all - used_racks



> >   end
-------- Original-Nachricht --------
Datum: Tue, 13 Jun 2006 19:08:13 +0900
Von: Robin S. <removed_email_address@domain.invalid>
An: removed_email_address@domain.invalid
Betreff: Re: ugly ruby code...
Peter E. (Guest)
on 2006-06-13 12:26
(Received via mailing list)
little typo (forget dot):

old: used_racks = Room.find(params[:id])racks
new: used_racks = Room.find(params[:id]).racks

-------- Original-Nachricht --------
Datum: Tue, 13 Jun 2006 19:22:37 +0900
Von: Peter E. <removed_email_address@domain.invalid>
An: removed_email_address@domain.invalid
Betreff: Re: ugly ruby code...
49ba272c5adeec974d54990765672738?d=identicon&s=25 Arnaud S. (Guest)
on 2006-06-13 13:52
Robin S. wrote:
> arnaud stageman wrote:
>>       @free_racks << rack
>>     end
>>   end
>> end
>
> Hi again,
>
> Just some minor suggestions:
>
> You seem to use only instance variables. If you are not going to need
> them later in views, I would prefer using local variables because they
> go out of scope when the method finishes and don't clutter the object
> with unnecessary variables. So, just replace @racks with racks.
>
> Replace the long Array.new with the shorter form []. Of course this is
> just my opinion, but I find it easier to type and easier to read as it
> gives you some kind of visual clue that there's an array involved.
>

Yes you're right I use now racks instead of @racks, thanks.

> ...my 0.02 Swiss francs ;)

Humm well in euro... Outch my head hurt :)
Kroeger, Simon (ext) (Guest)
on 2006-06-13 13:56
(Received via mailing list)
>
> Because you avoid sql injection.

From the docs:

"The array form is to be used when the condition input is
tainted and requires sanitization"

I wouldn't think of an id derived from another table as been tainted.
Perhaps I'm wrong, but please explain if this is the case. I don't think
you can store malicious code in an NUMERIC column?

> The first solution is better because you execute only one sql request.

I don't buy that without seen benchmark results. Iterating in pure ruby
while creating two new arrays (increasing the size each iteration) is
hardly faster than executing an SQL statement. (those db guys are realy
speed freaks :))

cheers

Simon
49ba272c5adeec974d54990765672738?d=identicon&s=25 Arnaud S. (Guest)
on 2006-06-13 14:19
Kroeger, Simon (ext) wrote:
>>
>> Because you avoid sql injection.
>
> From the docs:
>
> "The array form is to be used when the condition input is
> tainted and requires sanitization"
>
> I wouldn't think of an id derived from another table as been tainted.
> Perhaps I'm wrong, but please explain if this is the case. I don't think
> you can store malicious code in an NUMERIC column?
>

Honestly I don't have any clue about the good and bad method but it
costs nothing to do I believe :)

>> The first solution is better because you execute only one sql request.
>
> I don't buy that without seen benchmark results. Iterating in pure ruby
> while creating two new arrays (increasing the size each iteration) is
> hardly faster than executing an SQL statement. (those db guys are realy
> speed freaks :))
>

Yes you're right. I had not see the problem like that... It would need
further investigations.
As my app will be used in really small production environment, I think
I'll get your solution 'cause I find it more clear!
Peter E. (Guest)
on 2006-06-13 14:31
(Received via mailing list)
> > you can store malicious code in an NUMERIC column?

NULL, -1, 99999999999999999999999999999, 0

I could imagine all of these could cause trouble depending on db, etc.


-------- Original-Nachricht --------
Datum: Tue, 13 Jun 2006 21:20:21 +0900
Von: arnaud stageman <removed_email_address@domain.invalid>
An: removed_email_address@domain.invalid
Betreff: Re: ugly ruby code...
Dmitrii D. (Guest)
on 2006-06-13 14:31
(Received via mailing list)
> "The array form is to be used when the condition input is
> tainted and requires sanitization"
>
> I wouldn't think of an id derived from another table as been tainted.
> Perhaps I'm wrong, but please explain if this is the case. I don't think
> you can store malicious code in an NUMERIC column?

This code will one day be copied over to some other place where this
id is tainted. Then you'l have nightmare on yur hands :)
9c256437c358ebd784622906180d1773?d=identicon&s=25 Lou S. (Guest)
on 2006-06-13 14:38
(Received via mailing list)
On 6/13/06, Kroeger, Simon (ext) <removed_email_address@domain.invalid> wrote:
> I wouldn't think of an id derived from another table as been tainted.
> Perhaps I'm wrong, but please explain if this is the case. I don't think
> you can store malicious code in an NUMERIC column?

Well, in this case you are probably correct in assuming that that
particular data is safe, but it's not necessarily the case.  That
would be okay given that you check all your data _before_ you store it
in the database, but I wouldn't rely on the database for type safety.
What if you're using SQLite w/o strict affinity mode (which is the
default)?  If you don't do any validation in your application and you
just try to stick in that column, SQLite will be happy to do it for
you.  You might not be so happy of course =)
This topic is locked and can not be replied to.