Em 19-04-2012 20:01, Jeremy Evans escreveu:
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.
Oh, I see, I didn't realize that before.
...
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:
I didn't know this method didn't exist in 1.8. This is a valid argument.
Then, maybe inject:
initial_values.inject({}) do |h, (column, value)|
h[column] = [value, send(column)]; h
end
$ 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 don't really think this would make any noticeable difference in real
code. What is the code of hash_bench.rb? How much iterations is it doing?
I'm usually concerned more about code readability than negligible
performance improvements. Whenever I read lines like "h = {}" and "h",
something doesn't feel right to me, but I agree that this is entirely my
personal point of the view in the subject.
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.
But I don't think someone would call initial_values[:inexistent_column].
If that happens, it is most probably a code mistake and it should really
raise an exception.
Also, with your method, if you just want to get the cached value,
you have to do fetch(:foo, nil) instead of [:foo].
Do you have any real use case where only fetching the cached value would
be useful in this particular plugin?
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.
Thank you very much for your effort on this plugin. I'll really
appreciate when I'm finally able to use it from an official released gem :)
Have a great night,
Rodrigo.
--
You received this message because you are subscribed to the Google Groups
"sequel-talk" group.
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.
