update variables based on own value - critique please?

[Peter Ehrlich]

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

 

[Intransition]

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

 

[Josh Cheek]

[Note: parts of this message were removed to make it a legal post.]

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

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

 

[Peter Ehrlich]

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

 

[Josh Cheek]

[Note: parts of this message were removed to make it a legal post.]

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.

 

[Robert Klemme]

Hey!

First, some background. =A0The goal here is an experiment to make the bes= t
code that I can. =A0My challenge is with returning JSON in an ajax
request. =A0In a standard http request, rails view templates would handle
preparing the data -- pluralizing, capitalizing, whatever. =A0However,
there's no view in this case.

The simple approach looks like this:

=A0def json_for_view
=A0 =A0rounds['last_pushed']['c_score'] =3D time_ago_in_words
DateTime.parse(rounds[:last_pushed][:c_score])
=A0 =A0rounds['last_pushed']['d_score'] =3D time_ago_in_words
DateTime.parse(rounds[:last_pushed][:d_score])
=A0 =A0rounds['last_pushed']['c_score'] =3D Date.parse
rounds[:c_score].to_s'January 1, 2011')
=A0 =A0rounds['last_pushed']['d_score'] =3D Date.parse
rounds[:d_score].to_s'January 1, 2011')
=A0 =A0rounds.to_json
end

I see a couple of problems here:
=A0- the field keys are specified first. =A0This is not DRY. =A0(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] =3D time_ago_in_words
DateTime.parse(rounds[:last_pushed][key.to_sym])
lp[key] =3D Date.parse rounds[key.to_sym].to_s'January 1, 2011')
end
end

rounds.to_json
end

=A0- 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.

Here's what I've come up with:



# lib/toolbox.rb
class Object
=A0def send_updates!(hash)
=A0 =A0# updates values based on blocks passed in a hash

=A0 =A0hash.each do |key, block|
=A0 =A0 =A0 =A0self[key] =3D block.call(self[key]) if self[key]
=A0 =A0end

=A0end
end


# the model file
=A0def json_for_view
=A0 =A0rounds =3D JSON.parse(self.rounds)

=A0 =A0pushed_pretty =A0=3D lambda { |score|
time_ago_in_words(DateTime.parse(score)) =A0 =A0}
=A0 =A0created_pretty =3D lambda { |score| Date.parse(score).to_s'Janua= ry
1, 2011') }

=A0 =A0rounds['last_pushed'].send_updates!({
=A0 =A0 =A0'c_score' =3D> pushed_pretty,
=A0 =A0 =A0'd_score' =3D> pushed_pretty
=A0 =A0})
=A0 =A0rounds['created_at'].send_updates!({
=A0 =A0 =A0'c_score' =3D> created_pretty,
=A0 =A0 =A0'd_score' =3D> created_pretty
=A0 =A0})

=A0 =A0rounds.to_json
=A0end



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] =3D time_ago_in_words DateTime.parse(rd[:last_pushed][key.t=
o_sym])
lp[key] =3D Date.parse rd[key.to_sym].to_s'January 1, 2011')
end
end
end.to_json
end

Kind regards

robert

--=20
remember.guy do |as, often| as.you_can - without end
http://blog.rubybestpractices.com/