On Thursday, April 19, 2012 3:21:59 PM UTC-7, Rodrigo Rosenfeld Rosas wrote:
> I don't know enough yet about Sequel base code, but I didn't understand
> why you changed the first part already after starting to read it:
>
> v = typecast_value(column, value)
> vals = @values
> if new? || !vals.include?(column) || v != (c = vals[column]) ||
> v.class != c.class
> - changed_columns << column unless
> changed_columns.include?(column)
> - vals[column] = v
> + change_column_value(column, v)
> end
> end
>
> @@ -1702,6 +1701,13 @@ module Sequel
> use_transaction?(opts) ?
> db.transaction({:server=>this_server}.merge(opts)){yield} : yield
> end
>
> + # Change the value of the column to given value, recording the
> change.
> + def change_column_value(column, value)
> + cc = changed_columns
> + cc << column unless cc.include?(column)
> + @values[column] = value
> + end
>
> For instance, you're no longer calling "typecast_value". Is that
> intentional?
>
It's still being called inside []=, and the typecasted value is passed to
change_column_value. Yes, it's intentional, and it shouldn't change
existing behavior.
Also, particularly I don't see any reasons for changing
> changed_columns << column unless changed_columns.include?(column)
> to
> cc = changed_columns
> cc << column unless cc.include?(column)
>
This is an optimization. You do the method call once and cache it in a
local variable, so the second access uses the local variable instead of
another method call.
$ ruby19 ../lv_bench.rb
user system total real
cache in local variable 2.170000 0.000000 2.170000 ( 2.167999)
no caching 3.070000 0.000000 3.070000 ( 3.063186)
> def column_changes
> h = {}
> initial_values.each do |column, value|
> h[column] = [value, send(column)]
> end
> h
> end
>
> This is just a matter of style, but I'd rather write this as
>
> initial_values.each_with_object do |(column, value), h|
> h[column] = [value, send(column)]
> end
>
For one, each_with_object isn't available in 1.8.7:
$ ruby18 -ve '{}.each_with_object({}){}'
ruby 1.8.7 (2012-02-08 patchlevel 358) [i386-openbsd]
-e:1: undefined method `each_with_object' for {}:Hash (NoMethodError)
Benchmarking both ways shows that my code is measurably faster:
$ ruby19 ../hash_bench.rb
user system total real
each 2.950000 0.020000 2.970000 ( 2.967181)
each_with_object 3.530000 0.000000 3.530000 ( 3.529325)
> I've not tested this other style suggestion, but I guess it would work:
>
> def initial_value(column)
> initial_values.fetch(column){send(column)}
> end
>
> def initial_values
> @initial_values ||= {}
> end
>
> You wouldn't need the fetch with block if you changed initial_values to
> (again, untested):
>
> @initial_values ||= Hash.new{|h, k| send(k) } # or h[k] = send(k), if
> that is your intention
>
Using a default proc for a hash should be done sparingly. With your code:
initial_values[:foo]
raises an exception if the instance does not respond to the foo method,
whereas in my code, you just get nil, which is what most rubyists would
expect for a hash. Also, with your method, if you just want to get the
cached value, you have to do fetch(:foo, nil) instead of [:foo].
> Also, you wouldn't need this logic:
>
> value = if initial_values.has_key?(column)
> initial_values[column]
> else
> send(column)
> end
>
> Just calling initial_values[column] would suffice.
>
True, that's slightly nicer to read. But I prefer the existing code's
behavior.
> Also, I'd prefer to read
>
> initial_values[column] = value.clone rescue value
>
While this style is used occasionally in Sequel, it's really a bad idea and
I'm trying to move away from it. In general, you should rescue only the
exceptions you expect could be raised. If clone raises a non-TypeError,
that's probably a problem that should be fixed in that method. Swallowing
unexpected exceptions like this makes debugging more difficult later. I've
been bitten by such rescue clauses multiple times.
> or
>
> initial_values[column] = value.respond_to?(:clone) ? (value.clone rescue
> value) : value
>
> instead of
>
> initial_values[column] = if value.respond_to?(:clone)
> begin
> value.clone
> rescue TypeError
> value
> end
> else
> value
> end
>
> Is there any reason for checking only for TypeError? I'm guessing some
> frozen string, maybe. But this is not clear what you're protecting against
> and it makes it more difficult to read the code.
>
In general, attempting to clone an unclonable object in ruby raises a
TypeError:
$ ruby -e 'nil.clone'
-e:1:in `clone': can't clone NilClass (TypeError)
from -e:1:in `<main>'
Personally, I don't think an object should respond to clone if it's just
going to raise a TypeError if you call it, but that's just one of ruby's
warts.
> The specs and API seem fine to me.
>
> I'm sorry, I know I was not much useful here, but as I said, I don't know
> much about Sequel internals to catching some exceptional problematic
> situations, so I just gave the pastie a glance :P
>
> I'll appreciate this code integrated on next Sequel release :) Thank you
> very much, as always, you're very kind :)
>
I appreciate the code review. Thanks for your help. Hopefully I can
polish this up and commit it later today or tomorrow.
Jeremy
--
You received this message because you are subscribed to the Google Groups
"sequel-talk" group.
To view this discussion on the web visit
https://groups.google.com/d/msg/sequel-talk/-/8hrbxXaJ4D0J.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sequel-talk?hl=en.
