On Sun, May 1, 2011 at 3:44 AM, Peter E.
[email protected]wrote:
def json_for_view
end
end
By the way – is there any way to have json parse return keys as
symbols? I think that would be nice.
Cheers & Thanks,
–Peter
–
Posted via http://www.ruby-forum.com/.
TL;DR: Two similar lines isn’t enough to warrant all the complexity
you’ve
added 
def json_for_view
rounds[‘last_pushed’][‘c_score’] = time_ago_in_words
DateTime.parse(rounds[:last_pushed][:c_score])
rounds[‘last_pushed’][‘d_score’] = time_ago_in_words
DateTime.parse(rounds[:last_pushed][:d_score])
rounds[‘last_pushed’][‘c_score’] = Date.parse
rounds[:c_score].to_s(:‘January 1, 2011’)
rounds[‘last_pushed’][‘d_score’] = Date.parse
rounds[:d_score].to_s(:‘January 1, 2011’)
rounds.to_json
end
I’m confused, it looks like you’re setting
rounds[‘last_pushed’][‘c_score’],
and then immediately afterwards overriding what you initially set it to
(looking below, it appears that one of these was supposed to be
rounds[‘created_at’]). It also sounds like you are using Rails, and I
think
hashes in Rails have indifferent access, meaning
rounds[‘last_pushed’][‘c_score’] is the same as
rounds[:last_pushed][:c_score], so is the mixing of symbols and strings
intentional? Or are they actually different key/value pairs? Also, I’m
confused on the bottom two lines you access rounds[:c_score] directly,
but
everywhere else, its namespaced underneath rounds[:last_pushed] or
rounds[:created_at]. Is that intentional?
lib/toolbox.rb
class Object
def send_updates!(hash)
updates values based on blocks passed in a hash
hash.each do |key, block|
self[key] = block.call(self[key]) if self[key]
end
end
end
I don’t think its a good idea to declare public methods on Object,
unless
you really mean that every object should have that method (which is
pretty
rare). Lets say you do this a lot, and later you need to email all your
users of updates to their accounts, so you make a send_updates! method
on
Object. Boom!
I think you could just make this a private method in your model. (or if
it
needs to be used in several places, put it into a module and then
include it
where necessary)
the model file
def json_for_view
rounds = JSON.parse(self.rounds)
pushed_pretty = lambda { |score|
time_ago_in_words(DateTime.parse(score)) }
created_pretty = lambda { |score| Date.parse(score).to_s(:‘January 1,
2011’) }
rounds[‘last_pushed’].send_updates!({
‘c_score’ => pushed_pretty,
‘d_score’ => pushed_pretty
})
rounds[‘created_at’].send_updates!({
‘c_score’ => created_pretty,
‘d_score’ => created_pretty
})
rounds.to_json
end
Hmm, we went from 7 lines to 24, introduced callbacks, and made the code
generally pretty complex to follow. DRY is good, but this might be a bit
overzealous.
The real principle of DRY isn’t that two lines shouldn’t have similar
patterns, it’s “Every piece of knowledge must have a single,
unambiguous,
authoritative representation within a system”. In other words, if you do
this all over the place, and then you change it, you’ll have to remember
everywhere you did it, and go update that code. Instead, if you just say
what you want to happen, and let some code be responsible for doing
that,
then you only have to update it in the canonical place.
Take a look at the replacement code, then. Is it DRY? No. The logic for
how
to get the pushed_pretty and created_pretty is still located in
json_for_view. The only thing you’ve exported is how to update the hash.
If
you’re using this all over the place, and that logic changes, you will
still
have to go update each location. In addition, there’s another more
general
principle you’re violating here: KISS 
So I’d say it was better before. If you want to refactor it, find out
what
the core ideas are in the code, and extract that to its own place, which
can
become the canonical way to represent that task. If this is the only
place
you are using it, it becomes useful as documentation. If lots of other
code
is doing something similar, then all the code that would have duplicated
instead delegate to it. A single source.
As a last thought, the section in the Pragmatic Programmer about this is
really good, and it comes up in many contexts in that book. Definitely
worth
the read. I think a way that I’ve found most useful is to think of it
not as
removing duplication, but as documenting your code. I look at your code,
and
I don’t know what it does. So imagine you added a comment to help me
figure
it out. Now imagine if you had a method with a name that resembled the
comment, and moved the code in there. With good variable and method
names
and a healthy amount of delegation, comments become mostly redundant.
note: when the last argument to a Ruby method is a hash, you can omit
the
curly braces. ie
rounds[‘created_at’].send_updates!({
‘c_score’ => created_pretty,
‘d_score’ => created_pretty
})
could become
rounds[‘created_at’].send_updates! ‘c_score’ => created_pretty,
‘d_score’ =>
created_pretty