Ugly ruby code

Ruby
1

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?


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
2

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 :slight_smile:

Regards,
Robin

3

Great! It’s working perfectly!
Hummm Ruby :slight_smile:

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 :slight_smile:

Regards,
Robin

4

-----Original Message-----
From: [email protected]
[mailto:[email protected]] 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

5

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 :wink:

6

Kroeger, Simon (ext) wrote:

-----Original Message-----
From: [email protected]
[mailto:[email protected]] 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.

7

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. [email protected]
An: [email protected]
Betreff: Re: ugly ruby code…

8

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 :wink:

Humm well in euro… Outch my head hurt :slight_smile:

9

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

10

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. [email protected]
An: [email protected]
Betreff: Re: ugly ruby code…

11

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 [email protected]
An: [email protected]
Betreff: Re: ugly ruby code…

12

“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 :slight_smile:

13

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 :slight_smile:

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!

14

On 6/13/06, Kroeger, Simon (ext) [email protected] 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 =)