Update variables based on own value - critique please?

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

Something looks wrong to me. Why aren’t you using methods rather than
lambdas?

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


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

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

Hey Josh!

Thanks for the detailed answer.

What you say makes sense for extending object – it is not an action to
be taken lightly. I will see to using mixins or finding other places to
keep the code.

I think I find myself agreeing with all of your philosophies, but then
see this as example of that philosophy put in to practice, not
otherwise.

This was my striving to make code self commenting and DRY. Thats why I
create the lambdas, so that they have names, so that they can be read.

Thats why they are in this function, and thats why its simplified for
readability - the point of the method is really really trying to stay
simple, so that someone doesn’t come along later and make it
complicated.

Yes the code I add is not insignificant, and yes its a new way of doing
things when an old way would have done just as well. But this is what
made rails great! Without this thinking, we would have no capitalize
method or the other numerous rails helpers. I’m trying to imitate THAT
style.

Sure, if you count the changes, that is more. But that is simple
minded. Lines should be counted not as what was written but as what
must be maintained in the future. My hope is that the Object extension
remains simple and never must be touched again, which makes this code
shorter.

And even if my json viewing code takes more lines, that’s ok, because
the code is simpler. Here there is the perfect point-in-case. In my
‘before’ example I made not one but two typos (which you pointed out),
and led the reader to futher wonder if quoted string were really
required. (It turns out they were but didn’t have to be, and we see our
first refactor already requiring changes in EVERY key.) I, like many,
have the blessing and the curse of reading very quickly; it took me
several rereads to find the errors you pointed out, and those errors
would have had the potential to stump me thoroughly upon an exception.

@Thomas. I wish I could answer that, but I can’t really be sure I’m
picturing what you are. Were you thinking methods in view helpers file?
Or in the model? Or defined exactly where the lambdas are? (in latter
case, there’s no effective difference) Whatever the case, I’m trying to
keep as much as possible out of the larger namespace as much as possible
here – that is, unless there is elsewhere they may be needed.

Thanks all,
–Peter

On Sun, May 1, 2011 at 10:17 PM, Peter E.
[email protected]wrote:

Yes the code I add is not insignificant, and yes its a new way of doing
things when an old way would have done just as well. But this is what
made rails great! Without this thinking, we would have no capitalize
method or the other numerous rails helpers. I’m trying to imitate THAT
style.

Well, Capitalize is part of the Ruby String class (
http://ruby-doc.org/core/classes/String.html#M001157), but even if Rails
had
added it, it is not added on Object. Capitalize makes sense on Strings,
not
on arrays or floats or sessions. In your case, you appear to always
expecting self to be a hash, so at the very least, isn’t it more
appropriate
in Hash than.

And while that is one school of thought, there are plenty of other
respected
people in the community who think it is problematic when done too
haphazardly, and a number of them who would argue that Rails fits this
category. I’d be interested if someone more familiar with Rails could
comment about whether the number of methods they added to core / stdlib
has
increased, decreased, or stayed the same.

Here’s a talk that presents the other perspective, you might appreciate
http://rubymanor.org/videos/unobtrusive_metaprogramming/

You need to be especially concerned about this if you are writing
library
code. If you are writing your own code for your own app, that is one
thing.
But if you are writing a gem or something, then your code may find its
way
into thousands of different environments, where your changes to the way
Ruby
works could break other people’s code (or theirs could break yours).

Sure, if you count the changes, that is more. But that is simple

minded. Lines should be counted not as what was written but as what
must be maintained in the future. My hope is that the Object extension
remains simple and never must be touched again, which makes this code
shorter.

Okay. How about put a comment in there that says something like “every
time
you edit this method, increment this count: 0” and then at some point in
the
future, go back and see what the count is. I suspect that any time you
make
an edit that would have required changing the original method, you will
also
be required to change one of your refactored methods.

On Sun, May 1, 2011 at 10:44 AM, Peter E. [email protected]
wrote:

def json_for_view

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 simple refactoring leads me to

def json_for_view
%w[c_score d_score].each do |key|
rounds[‘last_pushed’].tap do |lp|
# next line seems obsolete since the value is overwritten
lp[key] = time_ago_in_words
DateTime.parse(rounds[:last_pushed][key.to_sym])
lp[key] = Date.parse rounds[key.to_sym].to_s(:‘January 1, 2011’)
end
end

rounds.to_json
end

  • A nicer feel would be if there was a method applied to each value,
    like rounds[:last_pushed][:d_score].make_pretty

That will be hard since Hash (or whatever this is) members generally
do not need to know the container. But that knowledge would be
required to do what you describe above.

self[key] = block.call(self[key]) if self[key]
pushed_pretty = lambda { |score|
‘d_score’ => created_pretty
})

rounds.to_json
end

Thoughts?

Why the lambdas? You said they should be named but why don’t you just
define another method? Your current solution imposes object creation
overhead for each call and you create new lambdas with identical
content all the time. I’d rather

def pushed_pretty(score)

end

and use that directly. There is no reason for a lambda here unless
you want to change the algorithm or even pass it in via a block:

def json_for_view(&pushed_pretty)

end

Is this a common problem?

That fact the you need Date.parse and DateTime.parse irritates me.
Normally your data should be in an internal data structure in the
proper type. Parsing from and conversion to string are normally
only necessary at the interface (i.e. when preparing a reply to be
sent or parsing a request).

Is there a common solution?

Am I doing something wrong which causes this to be an issue?

I don’t see the definition of rounds. This means it’s returned via
some method, so this might be better than the code above:

def json_for_view
rounds.tap do |rd|
%w[c_score d_score].each do |key|
rd[‘last_pushed’].tap do |lp|
# next line seems obsolete since the value is overwritten
lp[key] = time_ago_in_words
DateTime.parse(rd[:last_pushed][key.to_sym])
lp[key] = Date.parse rd[key.to_sym].to_s(:‘January 1, 2011’)
end
end
end.to_json
end

Kind regards

robert

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs