Re: update variables based on own value - critique please?

On Sun, May 1, 2011 at 3:44 AM, Peter Ehrlich <[e-mail]>wrote:

> Hey!
>
> First, some background.  The goal here is an experiment to make the best
> code that I can.  My challenge is with returning JSON in an ajax
> request.  In a standard http request, rails view templates would handle
> preparing the data -- pluralizing, capitalizing, whatever.  However,
> there's no view in this case.
>
> The simple approach looks like this:
>
>  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 see a couple of problems here:
>  - the field keys are specified first.  This is not DRY.  (etc all the
> headaches of non dry code)
>  - A nicer feel would be if there was a method applied to each value,
> like rounds[:last_pushed][:d_score].make_pretty
>
>
> Here's what I've come up with:
>
>
>
> # 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
>
>
> # 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
>
>
>
> Thoughts?
>
> Is this a common problem?
>
> Is there a common solution?
>
> Am I doing something wrong which causes this to be an issue?
>
> 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